Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Walkthrough레거시 결제 클라이언트/DTO 삭제 및 토스페이먼츠 전용 클라이언트·재시도 정책·도메인·서비스(파사드·트랜잭션)·예외·역직렬화기·마이그레이션·광범위한 테스트 추가. AOP 로깅 대상 변경 및 설정(yml) 업데이트 포함. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Facade as PaymentFacadeService
participant Retry as RetryTemplate
participant TossClient as TosspaymentsClient
participant TossAPI as Tosspayments API
participant DB as Database
Client->>Facade: confirmPayment(ConfirmRequest)
Facade->>DB: saveTosspaymentsPayment()
DB-->>Facade: TosspaymentsPayment (NEED_APPROVE)
Facade->>Retry: execute(콜백)
loop 재시도(지수적 백오프)
Retry->>TossClient: confirmPayment(TosspaymentsConfirmRequest)
TossClient->>TossAPI: POST /v1/payments/confirm
alt 성공(200)
TossAPI-->>TossClient: TosspaymentsPaymentResponse
TossClient-->>Retry: response
Retry-->>Facade: response
else 오류/예외
TossAPI-->>TossClient: 오류/예외
TossClient-->>Retry: 예외 던짐
Retry->>Retry: 재시도 판단
end
end
Facade->>DB: validateTosspaymentsResult()/saveResult()/updateState()
DB-->>Facade: 완료/예외
Facade-->>Client: PaymentResponse or Exception
sequenceDiagram
participant Client as Client
participant Facade as PaymentFacadeService
participant TossClient as TosspaymentsClient
participant TossAPI as Tosspayments API
participant Tx as TosspaymentsTransactionService
participant DB as Database
Client->>Facade: cancelPayment(CancelRequest)
Facade->>DB: readByPaymentKey()
DB-->>Facade: TosspaymentsPayment
Facade->>TossClient: cancelPayment(paymentKey, request)
TossClient->>TossAPI: POST /v1/payments/{paymentKey}/cancel
alt 성공(200)
TossAPI-->>TossClient: TosspaymentsPaymentResponse
TossClient-->>Facade: response
Facade->>Tx: applyCancelResult(response)
Tx->>DB: updatePaymentState(CANCELED) && updateCancelInfo()
DB-->>Tx: updated
Tx-->>Facade: 완료
Facade-->>Client: 성공
else 오류
TossAPI-->>TossClient: 예외
TossClient-->>Facade: 예외
Facade->>DB: updatePaymentState(오류상태)
Facade-->>Client: Exception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 @unifolio0, 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! 이 Pull Request는 기존의 결제 시스템을 토스페이먼츠(Tosspayments) 기반으로 전면 리팩토링하여 결제 처리의 안정성과 확장성을 대폭 향상시키는 것을 목표로 합니다. 새로운 결제 시스템은 토스페이먼츠 API와의 직접적인 연동을 통해 보다 정확하고 상세한 결제 정보 관리를 가능하게 하며, 견고한 재시도 메커니즘과 세분화된 오류 처리를 통해 다양한 결제 실패 시나리오에 유연하게 대응합니다. 또한, 결제 관련 도메인 모델을 명확히 정의하고 중앙 집중식 서비스 로직을 도입하여 코드의 가독성과 유지보수성을 개선했습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 코드 통합의 밤, 결제 흐름 새로이 짜네. 토스페이먼츠, 빛을 발하다. Footnotes
|
Test Results 46 files 46 suites 1m 19s ⏱️ Results for commit d08c539. ♻️ This comment has been updated with latest results. |
![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 refactors the payment processing logic to integrate with Toss Payments directly, replacing a previous internal payment client. It introduces new domain entities, services, and external client configurations for Toss Payments, along with retry mechanisms and enhanced error handling. The changes also include updates to the token purchase and refund flows to use the new payment facade service. Overall, the changes improve the robustness and clarity of the payment integration.
Comment on lines
80
to
92
| public void validateTosspaymentsResult(String paymentKey, String orderId, Long totalAmount) { | ||
| if (!this.paymentKey.equals(paymentKey)) { | ||
| log.error("paymentKey 불일치 - 응답: {}, DB: {}", paymentKey, this.paymentKey); | ||
| throw new InternalServerErrorException(PaymentErrorMessage.PAYMENT_KEY_MISMATCH.getMessage()); | ||
| } | ||
| if (!this.orderId.equals(orderId)) { | ||
| log.error("orderId 불일치 - 응답: {}, DB: {}", orderId, this.orderId); | ||
| throw new InternalServerErrorException(PaymentErrorMessage.ORDER_ID_MISMATCH.getMessage()); | ||
| } | ||
| if (!this.totalAmount.equals(totalAmount)) { | ||
| log.error("totalAmount 불일치 - 응답: {}, DB: {}", totalAmount, this.totalAmount); | ||
| throw new InternalServerErrorException(PaymentErrorMessage.TOTAL_AMOUNT_MISMATCH.getMessage()); | ||
| } |
There was a problem hiding this comment.
The validateTosspaymentsResult method is critical for ensuring data integrity by comparing the payment details from the Toss Payments response with the stored payment information. Throwing InternalServerErrorException for mismatches is appropriate as these indicate a severe internal inconsistency.
| requestFactory.setConnectTimeout(3000); | ||
| requestFactory.setReadTimeout(15000); | ||
|
|
||
| String encodedSecretKey = Base64.getEncoder().encodeToString((tossPaymentsWidgetSecretKey + ":").getBytes()); |
There was a problem hiding this comment.
The tossPaymentsWidgetSecretKey is used directly to create the Authorization header. While it's encoded, ensuring this key is handled securely (e.g., not logged, properly restricted access) is crucial. Consider if there are any other places this key might be exposed.
Comment on lines
+18
to
+24
| public TosspaymentsPaymentResponse confirmPayment(TosspaymentsConfirmRequest request, String idempotencyKey) { | ||
| return restClient.post() | ||
| .uri("/v1/payments/confirm") | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .body(request) | ||
| .retrieve() | ||
| .body(TosspaymentsPaymentResponse.class); |
There was a problem hiding this comment.
The confirmPayment method correctly uses Idempotency-Key header, which is crucial for preventing duplicate transactions when retrying payment confirmations. This ensures that even if the request is sent multiple times, the payment is processed only once.
Comment on lines
+55
to
+64
| String idempotencyKey = UUID.randomUUID().toString(); | ||
| try { | ||
| TosspaymentsPaymentResponse tosspaymentsConfirmResponse = tosspaymentsConfirmRetryTemplate.execute( | ||
| context -> { | ||
| if (context.getRetryCount() > 0) { | ||
| log.warn("토스페이먼츠 결제 승인 재시도 {}회차, paymentKey = {}", | ||
| context.getRetryCount(), request.paymentKey()); | ||
| } | ||
| return tosspaymentsClient.confirmPayment(request.toTosspaymentsConfirmRequest(), | ||
| idempotencyKey); |
There was a problem hiding this comment.
The retry logic using tosspaymentsConfirmRetryTemplate is well-implemented. Logging the retry attempts is helpful for monitoring and debugging. This significantly improves the reliability of payment confirmations against transient network or server issues.
Comment on lines
+95
to
+98
| if ("IDEMPOTENT_REQUEST_PROCESSING".equals(code)) { | ||
| log.error("토스 결제 처리 중 상태 지속 (409), paymentKey = {}", tosspaymentsPayment.getPaymentKey()); | ||
| tosspaymentsPaymentService.updateState(tosspaymentsPayment.getId(), PaymentState.NEED_CANCEL); | ||
| return new InternalServerErrorException(PaymentServiceErrorMessage.CONFIRM_SERVER_ERROR.getMessage(), e); |
There was a problem hiding this comment.
Specifically handling IDEMPOTENT_REQUEST_PROCESSING (409 Conflict) by setting the state to NEED_CANCEL and throwing an InternalServerErrorException is a good strategy. This indicates that the payment is in an uncertain state and requires further action.
Comment on lines
+47
to
+48
| @MockitoBean | ||
| private PaymentFacadeService paymentFacadeService; |
There was a problem hiding this comment.
The PaymentFacadeService is now mocked using @MockitoBean, which is appropriate for isolating the controller tests from the actual payment service implementation.
Comment on lines
+109
to
+114
| given(paymentFacadeService.confirmPayment(any())).willReturn(new PaymentResponse( | ||
| "test_paymentKey", null, "test_orderId", "토큰 10개", "mId", "KRW", | ||
| "간편결제", 500L, 500L, null, null, null, null, null, null, null, null, | ||
| false, null, null, null, | ||
| new com.samhap.kokomen.payment.service.dto.EasyPay("카카오페이", null, null), | ||
| null, null, null)); |
There was a problem hiding this comment.
The given statement for paymentFacadeService.confirmPayment has been updated to return the new PaymentResponse DTO, correctly reflecting the changes in the payment service integration.
|
|
||
| TokenRefundRequest request = new TokenRefundRequest(RefundReasonCode.CHANGE_OF_MIND, null); | ||
| willDoNothing().given(paymentClient).refundPayment(any()); | ||
| willDoNothing().given(paymentFacadeService).cancelPayment(any()); |
There was a problem hiding this comment.
The willDoNothing().given(paymentFacadeService).cancelPayment(any()) correctly mocks the cancellation call to the new PaymentFacadeService, ensuring that the token refund test can proceed without actual external calls.
Comment on lines
+25
to
+28
| tosspayments: | ||
| widget-secret-key: test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6 | ||
| retry: | ||
| tosspayments: |
There was a problem hiding this comment.
The removal of the old payment configuration and the addition of tosspayments and retry configurations are necessary for the new Toss Payments integration in the test environment.
Comment on lines
+29
to
+32
| max-attempts: 3 | ||
| initial-interval: 500 | ||
| multiplier: 2.0 | ||
| max-interval: 2000 |
There was a problem hiding this comment.
The new tosspayments and retry configurations for the test environment are correctly set up, ensuring that the Toss Payments integration is properly configured for testing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 44
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`:
- Around line 28-29: The current info-level logging in TosspaymentsLoggingAspect
logs the full response (log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}",
methodName, stopWatch.getTotalTimeMillis(), result)), which can expose sensitive
payment data; change this to either use log.debug for the response or
sanitize/whitelist only non-sensitive fields before logging. Locate the logging
in TosspaymentsLoggingAspect (the log.info line referencing methodName,
stopWatch.getTotalTimeMillis(), and result) and replace it with a debug-level
log or transform result via a sanitizer/mapper that strips card details, payment
method, receipt URLs, and other PII, ensuring only safe fields are logged while
keeping timing info intact.
- Line 22: The current INFO-level call in TosspaymentsLoggingAspect logging full
request arguments (log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args))
risks exposing sensitive payment/PII data; change the log level to DEBUG or
redact sensitive fields before logging by replacing the log.info call with
either log.debug(...) or log.info(...) that logs a sanitized representation
returned from a new helper (e.g., maskSensitiveFields(args) or
sanitizeArgs(args)); implement maskSensitiveFields/sanitizeArgs to detect and
redact known keys (paymentKey, cardNumber, amount, user identifiers) and use
that sanitized value in the logging call so sensitive data is never emitted at
INFO level.
- Around line 18-31: The logTosspaymentsApiCall method currently calls
joinPoint.proceed() without guarding for exceptions so stopWatch.stop() and the
response/elapsed-time logging are skipped; wrap the joinPoint.proceed() call in
a try-catch-finally: start the StopWatch before proceed, in finally always stop
the StopWatch and log the elapsed time, in the catch log the error (including
exception message/stack) with methodName and elapsed ms, then rethrow the
exception so behavior is unchanged; update references: logTosspaymentsApiCall,
joinPoint.proceed, StopWatch, stopWatch.stop, and log.info.
In `@src/main/java/com/samhap/kokomen/global/config/RetryConfig.java`:
- Around line 12-22: Extract the four `@Value` fields from RetryConfig into a
dedicated `@ConfigurationProperties`(prefix = "retry.tosspayments") class (e.g.,
TossPaymentsRetryProperties) containing maxAttempts, initialInterval,
multiplier, and maxInterval, annotate it with `@Validated` if you need validation,
and register it (via `@ConfigurationPropertiesScan` or
`@EnableConfigurationProperties`). Then update RetryConfig to accept the new
properties bean via constructor injection and remove the four `@Value`
annotations/fields so configuration becomes type-safe and supports
validation/IDE autocomplete.
In
`@src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java`:
- Around line 97-103: The handler method
handleHttpMediaTypeNotSupportedException in GlobalExceptionHandler currently
returns HttpStatus.BAD_REQUEST (400); change it to return
HttpStatus.UNSUPPORTED_MEDIA_TYPE (415) so unsupported content types correctly
produce a 415 response (update the ResponseEntity.status(...) call to
HttpStatus.UNSUPPORTED_MEDIA_TYPE while keeping the existing log and
ErrorResponse body).
In `@src/main/java/com/samhap/kokomen/global/exception/NotFoundException.java`:
- Around line 5-9: Add a second constructor to NotFoundException that accepts
(String message, Throwable cause) to match the other exception types; implement
it to delegate to the superclass constructor on KokomenException and pass
HttpStatus.NOT_FOUND so callers can wrap underlying causes consistently (refer
to NotFoundException, KokomenException, and HttpStatus.NOT_FOUND).
In
`@src/main/java/com/samhap/kokomen/global/exception/PaymentServiceErrorMessage.java`:
- Around line 1-25: PaymentServiceErrorMessage is a payment-domain specific enum
that should be moved out of global.exception into a payment-focused package
(e.g., payment.service or payment.domain) or consolidated with the existing
PaymentErrorMessage to avoid duplication; relocate the
PaymentServiceErrorMessage enum file into the chosen package (or merge its
constants into PaymentErrorMessage), update all usages/imports referencing
PaymentServiceErrorMessage, and ensure the enum name and constant semantics
remain consistent (adjust references to PaymentServiceErrorMessage and
PaymentErrorMessage as needed) so exception handling remains coherent and
package cohesion is improved.
In
`@src/main/java/com/samhap/kokomen/global/infrastructure/ObjectToStringDeserializer.java`:
- Around line 12-23: The deserialize method in ObjectToStringDeserializer should
treat JSON null as Java null to avoid bypassing `@NotBlank`: add a check if
(node.isNull()) return null; and stop producing the literal "null" string. Also
remove the per-call new ObjectMapper creation and use node.toString() to produce
the JSON text for object/array cases (i.e., replace
mapper.writeValueAsString(node) with node.toString()). Keep these changes inside
the deserialize(JsonParser p, DeserializationContext ctxt) method and ensure
behavior for node.isTextual() remains unchanged.
In `@src/main/java/com/samhap/kokomen/payment/domain/ServiceType.java`:
- Around line 3-6: The enum ServiceType currently declares a single constant
INTERVIEW followed by a trailing comma and a standalone semicolon which is
unnecessary and may imply more constants will be added; remove the trailing
comma and the semicolon (or if you intend to keep the semicolon for future
fields/methods, add a clarifying comment) so update the ServiceType enum
declaration to simply declare INTERVIEW without the extra punctuation or add an
explanatory comment above ServiceType indicating intentional formatting for
future additions.
In `@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java`:
- Around line 80-93: The current validateTosspaymentsResult method logs the raw
paymentKey which may be sensitive; change the log call that references
paymentKey in validateTosspaymentsResult to log a masked version instead (e.g.,
replace all but last N chars with asterisks) by using a small helper (e.g.,
maskSensitiveKey or maskPaymentKey) and call that helper when passing paymentKey
to log.error while leaving the thrown error messages untouched; update only the
log.error invocation(s) that reference paymentKey so the rest of the validation
(orderId, totalAmount) remains the same.
- Around line 64-74: The TosspaymentsPayment constructor lacks entity-level
validation; update the TosspaymentsPayment(String paymentKey, Long memberId,
String orderId, String orderName, Long totalAmount, String metadata, ServiceType
serviceType) constructor to validate non-null for paymentKey, orderId, memberId
and totalAmount (and any other fields annotated nullable = false), and ensure
totalAmount is not negative or zero; on violation throw an
IllegalArgumentException (or a domain-specific exception) with a clear message
so invalid state is prevented before DB constraints. Use the field names
paymentKey, orderId, memberId, totalAmount and set state only after validations
pass.
In
`@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResult.java`:
- Around line 114-142: The TosspaymentsPaymentResult constructor has 22
parameters which is error-prone; replace it with a Builder: make the public
constructor private (or package-private), add a static inner class
TosspaymentsPaymentResult.Builder with fluent setter methods for each field
(e.g., tosspaymentsPayment(TosspaymentsPayment), type(PaymentType), mId(String),
totalAmount(Long), balanceAmount(Long), suppliedAmount(Long), vat(Long), etc.),
a build() method that validates required fields and constructs a
TosspaymentsPaymentResult instance, and expose a public static builder() factory
on TosspaymentsPaymentResult; then update all call sites that used the long
constructor to use
TosspaymentsPaymentResult.builder().field(...).field(...).build().
- Around line 144-152: Replace the six-parameter updateCancelInfo signature with
a single cancel-info DTO to improve readability and future extensibility: create
a CancelInfo (or similar) value object containing fields cancelReason,
canceledAt, easyPayDiscountAmount, lastTransactionKey, cancelStatus,
tosspaymentsStatus, change the TosspaymentsPaymentResult.updateCancelInfo method
to accept that DTO and assign its properties to this.cancelReason,
this.canceledAt, this.easyPayDiscountAmount, this.lastTransactionKey,
this.cancelStatus and this.tosspaymentsStatus, and update any callers to supply
the new DTO instead of six separate arguments.
In `@src/main/java/com/samhap/kokomen/payment/external/dto/Failure.java`:
- Around line 3-7: 리팩토링 제안: DTO 명명 일관성을 맞추기 위해 record Failure를 FailureResponse로
리네임하거나 프로젝트 컨벤션을 따르도록 조정하세요; 구체적으로 src/main/java/...에서 record Failure(...) 선언을
FailureResponse(...)로 변경하고, 이 타입을 참조하는 모든 위치(예: EasyPay, TosspaymentsCancel 등
중첩/외부 DTO의 필드, 생성자, 메서드 시그니처 및 JSON 매핑 어노테이션)를 찾아 함께 업데이트하여 컴파일 오류와 직렬화/역직렬화 문제를
방지하세요.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TossDateTimeDeserializer.java`:
- Around line 14-17: TossDateTimeDeserializer.deserialize currently drops offset
by calling offsetDateTime.toLocalDateTime(); update it to preserve the instant
by converting the OffsetDateTime to the target zone before extracting
LocalDateTime: use
offsetDateTime.atZoneSameInstant(ZoneId.of("Asia/Seoul")).toLocalDateTime() (and
add import for ZoneId), or alternatively change the DTO field type to
OffsetDateTime and return the parsed OffsetDateTime directly from deserialize to
avoid losing offset information.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsCancel.java`:
- Around line 6-19: Rename the DTO record TosspaymentsCancel to
TosspaymentsCancelResponse to follow Request/Response naming conventions; update
all references/usages (mappers, service method signatures, controllers, tests,
JSON serialization/deserialization spots) to the new name and adjust imports
accordingly, and ensure any mapping code that constructs or consumes
TosspaymentsCancel (e.g., places that deserialize into this type or return it
from methods) is updated so compilation and runtime behavior remain correct.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsConfirmRequest.java`:
- Around line 3-8: TosspaymentsConfirmRequest's amount is declared as boxed Long
which can be null and produce invalid API requests; either change the
declaration to primitive long or add a compact constructor on the record
(TosspaymentsConfirmRequest) that validates paymentKey, orderId and amount with
Objects.requireNonNull (or explicit null checks) and throws a clear
NPE/IllegalArgumentException if any are null so Jackson never serializes a null
amount for the TossPayments confirm call.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.java`:
- Around line 3-6: Add a compact constructor to the record
TosspaymentsPaymentCancelRequest that validates the cancelReason field is not
null or blank; inside the compact constructor (in
TosspaymentsPaymentCancelRequest) check cancelReason and throw an
IllegalArgumentException with a clear message if it is null or blank so the
record cannot be instantiated with an invalid cancelReason required by the Toss
Payments cancel API.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.java`:
- Line 39: TosspaymentsPaymentResponse 클래스에서 cancels 필드만 FQN(java.util.List)으로
선언되어 일관성이 떨어집니다 — 파일 상단에 import java.util.List;를 추가하고 필드 선언을
java.util.List<TosspaymentsCancel> cancels 대신 List<TosspaymentsCancel> cancels로
변경해 일관된 import 사용을 유지하세요 (참조: 클래스 TosspaymentsPaymentResponse, 필드 cancels).
- Around line 42-68: The toTosspaymentsPaymentResult method constructs a
TosspaymentsPaymentResult with 22 parameters and repeats this.easyPay()
null-check three times; refactor by extracting var easyPay = this.easyPay() (and
similarly var receipt = this.receipt(), var failure = this.failure() if helpful)
and use easyPay.provider(), easyPay.amount(), easyPay.discountAmount() only if
easyPay != null to remove duplicated checks, and consider replacing the
long-parameter constructor with a builder or factory for
TosspaymentsPaymentResult to improve readability and future maintenance.
In
`@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java`:
- Line 18: The class name TossPaymentsClientBuilder is inconsistent with other
types using the Tosspayments naming; rename the public class
TossPaymentsClientBuilder to TosspaymentsClientBuilder and update all
references/usages accordingly (constructor calls, factory references, imports,
tests) so they match existing types like TosspaymentsClient,
TosspaymentsPayment, and TosspaymentsPaymentResult; ensure the class declaration
symbol and any files or build metadata that reference TossPaymentsClientBuilder
are updated to TosspaymentsClientBuilder to maintain project-wide naming
consistency.
- Line 31: The Base64 encoding in TossPaymentsClientBuilder currently calls
getBytes() without a charset which uses the platform default; update the
encoding of tossPaymentsWidgetSecretKey when building encodedSecretKey to call
getBytes with StandardCharsets.UTF_8 (import if necessary) so the Base64 result
is deterministic across environments; modify the expression that computes
encodedSecretKey in the class to use
tossPaymentsWidgetSecretKey.getBytes(StandardCharsets.UTF_8) before Base64
encoding.
- Around line 16-22: 현재 TossPaymentsClientBuilder 클래스는 `@Getter로`
tossPaymentsClientBuilder(RestClient.Builder)를 외부에 노출하고 있어 공유 가변 상태 이슈가 발생합니다;
`@Getter를` 제거하고 tossPaymentsClientBuilder를 private으로 유지한 후 외부에 RestClient 인스턴스만
제공하도록 변경하세요 — 예: TossPaymentsClientBuilder에 public RestClient
createTossPaymentsClient() 혹은 getTossPaymentsClient() 메서드를 추가해 내부의
tossPaymentsClientBuilder로부터 새로운 RestClient를 매번 빌드해 반환하거나(빌더가 재사용될 수 있게 매 호출마다
새로운 Builder를 생성) 빌더를 안전하게 복제한 뒤 반환하도록 구현하고, TOSSPAYMENTS_API_URL과 관련된 설정은 이 생성
메서드 내부에서만 적용해 불변 객체만 외부로 노출되게 하세요 (참조: TossPaymentsClientBuilder,
tossPaymentsClientBuilder, RestClient.Builder, TOSSPAYMENTS_API_URL).
In
`@src/main/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentResultRepository.java`:
- Around line 7-9: The derived query method findByTosspaymentsPaymentId in
TosspaymentsPaymentResultRepository is incorrect because the
TosspaymentsPaymentResult entity exposes a relationship field named
tosspaymentsPayment (not tosspaymentsPaymentId); rename the repository method to
findByTosspaymentsPayment_Id(Long id) so Spring Data traverses the relation (or
alternatively add an `@Query` on TosspaymentsPaymentResultRepository that selects
by the foreign key column), ensuring you update the method signature in
TosspaymentsPaymentResultRepository accordingly.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/Checkout.java`:
- Around line 3-9: Rename the DTO record Checkout to CheckoutResponse and update
its file name and all usages; specifically change the record declaration symbol
Checkout to CheckoutResponse, update the static factory method
from(com.samhap.kokomen.payment.external.dto.Checkout) to return new
CheckoutResponse(checkout.url()), and replace all imports/usages of
com.samhap.kokomen.payment.service.dto.Checkout throughout the codebase to
com.samhap.kokomen.payment.service.dto.CheckoutResponse (including tests,
controllers, mappers and serializers) so the DTO follows the Request/Response
naming convention.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/EasyPay.java`:
- Around line 3-15: The DTO record EasyPay violates naming rules—rename the
record EasyPay to EasyPayResponse and update the file name accordingly; adjust
the static factory method from(com.samhap.kokomen.payment.external.dto.EasyPay
easyPay) to return new EasyPayResponse(...) and update all usages/imports
referencing EasyPay (including callers, tests, and any mapping code) to use
EasyPayResponse so the codebase matches the Request/Response DTO naming
convention.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/Failure.java`:
- Around line 3-10: Rename the DTO and mapping method to follow naming rules:
change the record name Failure to a Request/Response-suffixed name (e.g.,
FailureResponse) and replace the static from(...) method with an action+domain
name such as mapFailure(...) or createFailure(...); update the signature to
static FailureResponse
mapFailure(com.samhap.kokomen.payment.external.dto.Failure failure) and return a
new FailureResponse(failure.code(), failure.message()); then update all call
sites to the new type and method name (referencing the record Failure ->
FailureResponse and method from -> mapFailure).
In `@src/main/java/com/samhap/kokomen/payment/service/dto/PaymentResponse.java`:
- Around line 23-34: Remove the unnecessary Jackson deserialization annotations
from PaymentResponse: delete `@JsonDeserialize`(using =
TossDateTimeDeserializer.class) on requestedAt and approvedAt and
`@JsonDeserialize`(using = ObjectToStringDeserializer.class) on metadata;
PaymentResponse is only created via the from() factory that accepts a
TosspaymentsPaymentResponse, so keep the fields as plain LocalDateTime/ String
and ensure the from() method remains the single construction path.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/Receipt.java`:
- Around line 3-9: The DTO record Receipt violates naming rules — rename the
record Receipt to ReceiptResponse and update the factory method and its
signature: change the record declaration (currently "public record Receipt") to
"public record ReceiptResponse", update the static factory method from(...) to
return a new ReceiptResponse (adjusting its return type and constructor call),
and update all usages/imports/references across the codebase (including the
external mapping in from(com.samhap.kokomen.payment.external.dto.Receipt) and
any callers) so the file name and all references reflect ReceiptResponse.
In
`@src/main/java/com/samhap/kokomen/payment/service/dto/TosspaymentsCancel.java`:
- Around line 7-35: Rename the record TosspaymentsCancel to
TosspaymentsCancelResponse and rename the static factory method
from(com.samhap.kokomen.payment.external.dto.TosspaymentsCancel cancel) to
mapExternalCancel(com.samhap.kokomen.payment.external.dto.TosspaymentsCancel
cancel); update the returned constructor call to construct a
TosspaymentsCancelResponse with the same fields and ensure any call sites
referencing TosspaymentsCancel or from(...) are updated to use
TosspaymentsCancelResponse and mapExternalCancel(...).
In `@src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java`:
- Around line 112-124: handleConfirmServerError: add an explicit null check
after calling e.getResponseBodyAs(TosspaymentsPaymentResponse.class) to avoid an
NPE before invoking
tosspaymentsConfirmResponse.toTosspaymentsPaymentResult(...); if the response is
null, log a warning (including payment id and that response was null) and call
tosspaymentsPaymentService.updateState(tosspaymentsPayment.getId(),
PaymentState.NEED_CANCEL) instead of proceeding, otherwise continue converting
and call tosspaymentsTransactionService.applyTosspaymentsPaymentResult(...) as
before.
In
`@src/main/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionService.java`:
- Around line 34-43: The code in TosspaymentsTransactionService currently picks
the first TosspaymentsCancel (response.cancels().get(0)), which can omit the
latest cancel in multi-cancel scenarios; change the logic to choose the latest
cancel (either use the list's last element or select the TosspaymentsCancel with
the greatest canceledAt) and pass that cancel into result.updateCancelInfo along
with response.lastTransactionKey() and response.status(); ensure you handle
null/empty cancels() safely before accessing the chosen element.
In `@src/main/java/com/samhap/kokomen/token/dto/TokenPurchaseRequest.java`:
- Around line 28-35: The code currently uses TokenProduct.valueOf(productName)
in toPaymentConfirmRequest (and another method around lines 54-56) which throws
IllegalArgumentException and surfaces as 500; add a private helper
readTokenProduct(String productName) that calls
TokenProduct.valueOf(productName) and catches IllegalArgumentException to
rethrow a BadRequestException with a clear message, then replace direct
TokenProduct.valueOf usages in toPaymentConfirmRequest and the other method with
readTokenProduct to ensure invalid product_name returns HTTP 400.
In `@src/main/java/com/samhap/kokomen/token/service/TokenFacadeService.java`:
- Around line 47-51: The DTO method request.toPaymentConfirmRequest(memberId,
objectMapper) makes the DTO depend on ObjectMapper; move the mapping logic out
of the DTO into the service layer or a dedicated mapper class (e.g., create a
PaymentConfirmRequestMapper with a method toBuildConfirmRequest(request,
memberId, objectMapper) or have TokenFacadeService perform the conversion using
objectMapper), then call paymentFacadeService.confirmPayment with the produced
ConfirmRequest and continue using request.toTokenPurchase only for pure data
conversion; update TokenFacadeService to call the new mapper/inline mapping
instead of request.toPaymentConfirmRequest and remove ObjectMapper usage from
the DTO.
- Around line 49-51: The inline ternary for easyPay is hard to read and risks
NPE; extract paymentResponse.easyPay() into a local variable or use Optional to
compute provider before calling request.toTokenPurchase. For example, assign
EasyPay easyPay = paymentResponse.easyPay(); compute String provider = easyPay
!= null ? easyPay.provider() : null (or use
Optional.ofNullable(paymentResponse.easyPay()).map(EasyPay::provider).orElse(null)),
then pass provider into request.toTokenPurchase when calling
tokenPurchaseService.saveTokenPurchase to improve readability and null-safety.
In `@src/main/resources/application-local.yml`:
- Around line 22-23: Replace the hardcoded tosspayments.widget-secret-key value
with an environment-variable-backed property so the secret is injected at
runtime; specifically, update the application-local.yml entry for
tosspayments.widget-secret-key to reference an environment variable (e.g.,
WIDGET_SECRET_KEY or WIDGET_SECRET_KEY_LOCAL) using Spring Boot property syntax
with the current public test key as the fallback, matching the pattern used in
application-prod.yml (which uses ${WIDGET_SECRET_KEY_PROD}); ensure no literal
key remains committed so CI/Gitleaks won't flag it and local fallback continues
to work.
In `@src/main/resources/db/migration/V39__create_tosspayments_payment_tables.sql`:
- Line 48: The canceled_at column uses DATETIME (second precision) while
created_at and updated_at use DATETIME(6); update the schema so canceled_at is
declared as DATETIME(6) to match created_at/updated_at and preserve microsecond
precision when mapping to JPA LocalDateTime (change the canceled_at column
definition to DATETIME(6) DEFAULT NULL).
In
`@src/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java`:
- Around line 39-77: The builder has extra setter methods that aren’t used by
current tests; in TosspaymentsPaymentResultFixtureBuilder remove the unnecessary
setters (type, method, tosspaymentsStatus, approvedAt, receiptUrl, failureCode,
failureMessage) and keep only tosspaymentsPayment(...) (and build()), relying on
the build() method’s existing default values for totalAmount, balanceAmount,
isPartialCancelable, etc.; ensure build() still composes the fixture correctly
after removing these methods.
- Around line 88-90: The builder currently defaults requestedAt to
LocalDateTime.now().minusMinutes(5), making tests time-dependent; change the
default in TosspaymentsPaymentResultFixtureBuilder so that when requestedAt is
null it uses a fixed LocalDateTime constant (e.g. FIXED_REQUESTED_AT) instead of
now(), update the ternary where requestedAt is set (the expression using
tosspaymentsStatus != null ? ... , requestedAt != null ? requestedAt :
LocalDateTime.now().minusMinutes(5), approvedAt) to use that fixed constant, and
add a private static final LocalDateTime FIXED_REQUESTED_AT field so tests are
deterministic.
In
`@src/test/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentTest.java`:
- Around line 26-28: Replace direct TosspaymentsPayment constructor usages in
TosspaymentsPaymentTest with the existing TosspaymentsPaymentFixtureBuilder to
reduce duplication and centralize fixture construction; locate each
instantiation of TosspaymentsPayment (e.g., the instance created with
"payment_key", 1L, "order_id", "주문명", 10000L, "{}", ServiceType.INTERVIEW and
the ones at the other mentioned ranges) and construct them via
TosspaymentsPaymentFixtureBuilder by invoking the builder methods to set
paymentKey, memberId, orderId, name, amount, metadata, and serviceType as
needed, then build() so future constructor signature changes only require
updating the fixture builder.
In
`@src/test/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionServiceTest.java`:
- Around line 33-100: Both test methods duplicate the setup of a completed
TosspaymentsPayment and its TosspaymentsPaymentResult; extract that into a
private helper (e.g., createCompletedPaymentWithResult or setUpCompletedPayment)
inside TosspaymentsTransactionServiceTest to return the saved
TosspaymentsPayment (or its id) and associated TosspaymentsPaymentResult. The
helper should call TosspaymentsPaymentFixtureBuilder.builder(), set paymentKey,
call payment.updateState(PaymentState.COMPLETED), persist with
tosspaymentsPaymentRepository.save(payment), create and persist the
TosspaymentsPaymentResult via TosspaymentsPaymentResultFixtureBuilder and
tosspaymentsPaymentResultRepository.save(result); then replace the duplicated
blocks in both tests with calls to this helper.
- Around line 33-69: The test 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() omits
asserting that lastTransactionKey was updated from the response; after calling
tosspaymentsTransactionService.applyCancelResult(response) and loading
updatedResult (via findByTosspaymentsPaymentId), add an assertion that
updatedResult.getLastTransactionKey() equals the "cancel_tx_key" provided in the
TosspaymentsPaymentResponse so the cancel flow (applyCancelResult) properly
updates lastTransactionKey.
In `@src/test/java/com/samhap/kokomen/token/controller/TokenControllerTest.java`:
- Around line 42-48: The test currently mocks the service layer (remove the
`@MockitoBean` on PaymentFacadeService in TokenControllerTest) which violates the
integration-test-with-MockMvc guideline; instead keep PaymentFacadeService as
the real Spring bean and mock only the external payment client (e.g.,
TosspaymentsClient), moving any stubbing that targets PaymentFacadeService to
stub the TosspaymentsClient responses so MockMvc exercises the real
service+controller wiring and REST Docs generation still works; apply the same
change to the other occurrences referenced (the other `@MockitoBean` usages around
the file).
In `@src/test/resources/application.yml`:
- Around line 25-26: 현재 테스트 리소스에 하드코딩된 secret 값(widget-secret-key)이 포함되어 있으니 이를
제거하고 환경변수 주입으로 변경하세요: application.yml의 widget-secret-key 항목을 하드코딩된 값 대신 플레이스홀더로
바꿔 예: widget-secret-key: ${WIDGET_SECRET_KEY:dummy_widget_secret} 처럼 기본 더미 값을
제공하고, 테스트/CI에서는 필요한 경우 WIDGET_SECRET_KEY를 설정하도록 하며 실제 시크릿은 레포에 커밋하지 않도록 처리합니다.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (65)
src/main/java/com/samhap/kokomen/global/aop/PaymentLoggingAspect.javasrc/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.javasrc/main/java/com/samhap/kokomen/global/config/RetryConfig.javasrc/main/java/com/samhap/kokomen/global/config/TosspaymentsConfirmRetryPolicy.javasrc/main/java/com/samhap/kokomen/global/exception/BadRequestException.javasrc/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.javasrc/main/java/com/samhap/kokomen/global/exception/InternalServerErrorException.javasrc/main/java/com/samhap/kokomen/global/exception/NotFoundException.javasrc/main/java/com/samhap/kokomen/global/exception/PaymentServiceErrorMessage.javasrc/main/java/com/samhap/kokomen/global/infrastructure/ObjectToStringDeserializer.javasrc/main/java/com/samhap/kokomen/payment/domain/PaymentErrorMessage.javasrc/main/java/com/samhap/kokomen/payment/domain/PaymentState.javasrc/main/java/com/samhap/kokomen/payment/domain/PaymentType.javasrc/main/java/com/samhap/kokomen/payment/domain/ServiceType.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResult.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsStatus.javasrc/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.javasrc/main/java/com/samhap/kokomen/payment/external/TosspaymentsClient.javasrc/main/java/com/samhap/kokomen/payment/external/TosspaymentsInternalServerErrorCode.javasrc/main/java/com/samhap/kokomen/payment/external/dto/Checkout.javasrc/main/java/com/samhap/kokomen/payment/external/dto/EasyPay.javasrc/main/java/com/samhap/kokomen/payment/external/dto/Failure.javasrc/main/java/com/samhap/kokomen/payment/external/dto/Receipt.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TossDateTimeDeserializer.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsCancel.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsConfirmRequest.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.javasrc/main/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentRepository.javasrc/main/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentResultRepository.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/TosspaymentsPaymentService.javasrc/main/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionService.javasrc/main/java/com/samhap/kokomen/payment/service/dto/CancelRequest.javasrc/main/java/com/samhap/kokomen/payment/service/dto/Checkout.javasrc/main/java/com/samhap/kokomen/payment/service/dto/ConfirmRequest.javasrc/main/java/com/samhap/kokomen/payment/service/dto/EasyPay.javasrc/main/java/com/samhap/kokomen/payment/service/dto/Failure.javasrc/main/java/com/samhap/kokomen/payment/service/dto/PaymentResponse.javasrc/main/java/com/samhap/kokomen/payment/service/dto/Receipt.javasrc/main/java/com/samhap/kokomen/payment/service/dto/TosspaymentsCancel.javasrc/main/java/com/samhap/kokomen/token/dto/ConfirmRequest.javasrc/main/java/com/samhap/kokomen/token/dto/PaymentResponse.javasrc/main/java/com/samhap/kokomen/token/dto/RefundRequest.javasrc/main/java/com/samhap/kokomen/token/dto/TokenPurchaseRequest.javasrc/main/java/com/samhap/kokomen/token/external/PaymentClient.javasrc/main/java/com/samhap/kokomen/token/external/PaymentClientBuilder.javasrc/main/java/com/samhap/kokomen/token/service/TokenFacadeService.javasrc/main/resources/application-dev.ymlsrc/main/resources/application-load-test.ymlsrc/main/resources/application-local.ymlsrc/main/resources/application-prod.ymlsrc/main/resources/db/migration/V39__create_tosspayments_payment_tables.sqlsrc/test/java/com/samhap/kokomen/global/BaseTest.javasrc/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentFixtureBuilder.javasrc/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.javasrc/test/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResultTest.javasrc/test/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentTest.javasrc/test/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentRepositoryTest.javasrc/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.javasrc/test/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionServiceTest.javasrc/test/java/com/samhap/kokomen/token/controller/TokenControllerTest.javasrc/test/resources/application.yml
💤 Files with no reviewable changes (6)
- src/main/java/com/samhap/kokomen/token/external/PaymentClient.java
- src/main/java/com/samhap/kokomen/token/dto/PaymentResponse.java
- src/main/java/com/samhap/kokomen/token/dto/RefundRequest.java
- src/main/java/com/samhap/kokomen/global/aop/PaymentLoggingAspect.java
- src/main/java/com/samhap/kokomen/token/external/PaymentClientBuilder.java
- src/main/java/com/samhap/kokomen/token/dto/ConfirmRequest.java
Comment on lines
18
to
31
| public Object logTosspaymentsApiCall(ProceedingJoinPoint joinPoint) throws Throwable { | ||
| String methodName = joinPoint.getSignature().getName(); | ||
| Object[] args = joinPoint.getArgs(); | ||
|
|
||
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); | ||
|
|
||
| StopWatch stopWatch = new StopWatch(); | ||
| stopWatch.start(); | ||
| Object result = joinPoint.proceed(); | ||
| stopWatch.stop(); | ||
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
예외 발생 시 경과 시간 및 에러 로깅 누락
joinPoint.proceed()에서 예외가 발생하면 stopWatch.stop()과 응답 로그가 실행되지 않습니다. try-catch-finally 패턴을 적용하여 예외 시에도 경과 시간과 에러 정보를 로깅하면 운영 시 디버깅에 유용합니다.
♻️ 예외 처리 추가 제안
`@Around`("execution(* com.samhap.kokomen.payment.external.TosspaymentsClient.*(..))")
public Object logTosspaymentsApiCall(ProceedingJoinPoint joinPoint) throws Throwable {
String methodName = joinPoint.getSignature().getName();
Object[] args = joinPoint.getArgs();
log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args);
StopWatch stopWatch = new StopWatch();
stopWatch.start();
- Object result = joinPoint.proceed();
- stopWatch.stop();
- log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}",
- methodName, stopWatch.getTotalTimeMillis(), result);
- return result;
+ try {
+ Object result = joinPoint.proceed();
+ stopWatch.stop();
+ log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}",
+ methodName, stopWatch.getTotalTimeMillis(), result);
+ return result;
+ } catch (Throwable e) {
+ stopWatch.stop();
+ log.error("[토스페이먼츠 API 에러] {} - {}ms - error: {}",
+ methodName, stopWatch.getTotalTimeMillis(), e.getMessage());
+ throw e;
+ }
}📝 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 Object logTosspaymentsApiCall(ProceedingJoinPoint joinPoint) throws Throwable { | |
| String methodName = joinPoint.getSignature().getName(); | |
| Object[] args = joinPoint.getArgs(); | |
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); | |
| StopWatch stopWatch = new StopWatch(); | |
| stopWatch.start(); | |
| Object result = joinPoint.proceed(); | |
| stopWatch.stop(); | |
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | |
| methodName, stopWatch.getTotalTimeMillis(), result); | |
| return result; | |
| } | |
| public Object logTosspaymentsApiCall(ProceedingJoinPoint joinPoint) throws Throwable { | |
| String methodName = joinPoint.getSignature().getName(); | |
| Object[] args = joinPoint.getArgs(); | |
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); | |
| StopWatch stopWatch = new StopWatch(); | |
| stopWatch.start(); | |
| try { | |
| Object result = joinPoint.proceed(); | |
| stopWatch.stop(); | |
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | |
| methodName, stopWatch.getTotalTimeMillis(), result); | |
| return result; | |
| } catch (Throwable e) { | |
| stopWatch.stop(); | |
| log.error("[토스페이먼츠 API 에러] {} - {}ms - error: {}", | |
| methodName, stopWatch.getTotalTimeMillis(), e.getMessage()); | |
| throw e; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
around lines 18 - 31, The logTosspaymentsApiCall method currently calls
joinPoint.proceed() without guarding for exceptions so stopWatch.stop() and the
response/elapsed-time logging are skipped; wrap the joinPoint.proceed() call in
a try-catch-finally: start the StopWatch before proceed, in finally always stop
the StopWatch and log the elapsed time, in the catch log the error (including
exception message/stack) with methodName and elapsed ms, then rethrow the
exception so behavior is unchanged; update references: logTosspaymentsApiCall,
joinPoint.proceed, StopWatch, stopWatch.stop, and log.info.
| String methodName = joinPoint.getSignature().getName(); | ||
| Object[] args = joinPoint.getArgs(); | ||
|
|
||
| log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); |
There was a problem hiding this comment.
요청 인자를 INFO 레벨로 전체 로깅 시 민감 정보 노출 위험
args에는 결제 키, 금액, 사용자 정보 등 민감한 데이터가 포함될 수 있습니다. 전체 인자를 INFO 레벨로 로깅하면 로그 수집 시스템에 PII/결제 정보가 유출될 수 있습니다. DEBUG 레벨로 변경하거나, 민감 필드를 마스킹하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
at line 22, The current INFO-level call in TosspaymentsLoggingAspect logging
full request arguments (log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName,
args)) risks exposing sensitive payment/PII data; change the log level to DEBUG
or redact sensitive fields before logging by replacing the log.info call with
either log.debug(...) or log.info(...) that logs a sanitized representation
returned from a new helper (e.g., maskSensitiveFields(args) or
sanitizeArgs(args)); implement maskSensitiveFields/sanitizeArgs to detect and
redact known keys (paymentKey, cardNumber, amount, user identifiers) and use
that sanitized value in the logging call so sensitive data is never emitted at
INFO level.
Comment on lines
28
to
29
| log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", | ||
| methodName, stopWatch.getTotalTimeMillis(), result); |
There was a problem hiding this comment.
응답 객체 전체 로깅 시 민감 정보 노출 위험
응답에도 결제 수단, 카드 정보, 영수증 URL 등 민감한 데이터가 포함될 수 있습니다. 요청 인자와 동일하게 DEBUG 레벨 사용 또는 선택적 필드만 로깅하는 방안을 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`
around lines 28 - 29, The current info-level logging in
TosspaymentsLoggingAspect logs the full response (log.info("[토스페이먼츠 API 응답] {} -
{}ms - response: {}", methodName, stopWatch.getTotalTimeMillis(), result)),
which can expose sensitive payment data; change this to either use log.debug for
the response or sanitize/whitelist only non-sensitive fields before logging.
Locate the logging in TosspaymentsLoggingAspect (the log.info line referencing
methodName, stopWatch.getTotalTimeMillis(), and result) and replace it with a
debug-level log or transform result via a sanitizer/mapper that strips card
details, payment method, receipt URLs, and other PII, ensuring only safe fields
are logged while keeping timing info intact.
Comment on lines
+12
to
+22
| @Value("${retry.tosspayments.max-attempts}") | ||
| private int maxAttempts; | ||
|
|
||
| @Value("${retry.tosspayments.initial-interval}") | ||
| private long initialInterval; | ||
|
|
||
| @Value("${retry.tosspayments.multiplier}") | ||
| private double multiplier; | ||
|
|
||
| @Value("${retry.tosspayments.max-interval}") | ||
| private long maxInterval; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
@ConfigurationProperties 바인딩 사용 권장
4개의 @Value 어노테이션 대신 @ConfigurationProperties(prefix = "retry.tosspayments")를 사용한 별도 프로퍼티 클래스로 추출하면 타입 안전성, 유효성 검증(@Validated), IDE 자동완성 등의 이점을 얻을 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/global/config/RetryConfig.java` around lines
12 - 22, Extract the four `@Value` fields from RetryConfig into a dedicated
`@ConfigurationProperties`(prefix = "retry.tosspayments") class (e.g.,
TossPaymentsRetryProperties) containing maxAttempts, initialInterval,
multiplier, and maxInterval, annotate it with `@Validated` if you need validation,
and register it (via `@ConfigurationPropertiesScan` or
`@EnableConfigurationProperties`). Then update RetryConfig to accept the new
properties bean via constructor injection and remove the four `@Value`
annotations/fields so configuration becomes type-safe and supports
validation/IDE autocomplete.
src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java
Show resolved
Hide resolved
Comment on lines
+26
to
+28
| TosspaymentsPayment payment = new TosspaymentsPayment( | ||
| "payment_key", 1L, "order_id", "주문명", 10000L, "{}", ServiceType.INTERVIEW | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TosspaymentsPaymentFixtureBuilder 활용 고려
테스트 픽스처 빌더(TosspaymentsPaymentFixtureBuilder)가 이미 존재합니다. 반복되는 TosspaymentsPayment 생성자 호출을 빌더로 대체하면, 향후 생성자 시그니처 변경 시 수정 포인트를 줄일 수 있습니다.
Also applies to: 37-39, 48-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentTest.java`
around lines 26 - 28, Replace direct TosspaymentsPayment constructor usages in
TosspaymentsPaymentTest with the existing TosspaymentsPaymentFixtureBuilder to
reduce duplication and centralize fixture construction; locate each
instantiation of TosspaymentsPayment (e.g., the instance created with
"payment_key", 1L, "order_id", "주문명", 10000L, "{}", ServiceType.INTERVIEW and
the ones at the other mentioned ranges) and construct them via
TosspaymentsPaymentFixtureBuilder by invoking the builder methods to set
paymentKey, memberId, orderId, name, amount, metadata, and serviceType as
needed, then build() so future constructor signature changes only require
updating the fixture builder.
Comment on lines
+33
to
+100
| @Test | ||
| void 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() { | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("payment_key") | ||
| .build(); | ||
| payment.updateState(PaymentState.COMPLETED); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
|
|
||
| TosspaymentsPaymentResult result = TosspaymentsPaymentResultFixtureBuilder.builder() | ||
| .tosspaymentsPayment(payment) | ||
| .build(); | ||
| tosspaymentsPaymentResultRepository.save(result); | ||
|
|
||
| LocalDateTime canceledAt = LocalDateTime.of(2025, 1, 1, 12, 0); | ||
| TosspaymentsCancel cancel = new TosspaymentsCancel( | ||
| "cancel_tx_key", "단순 변심", 0L, | ||
| canceledAt, 0L, null, 10000L, 0L, 10000L, "DONE", null | ||
| ); | ||
| TosspaymentsPaymentResponse response = new TosspaymentsPaymentResponse( | ||
| "payment_key", PaymentType.NORMAL, "order_id", "주문명", | ||
| "tvivarepublica", "KRW", "카드", 10000L, 10000L, | ||
| TosspaymentsStatus.CANCELED, LocalDateTime.now(), LocalDateTime.now(), | ||
| "cancel_tx_key", 9091L, 909L, 0L, 0L, true, | ||
| "{}", null, null, null, "KR", null, List.of(cancel) | ||
| ); | ||
|
|
||
| tosspaymentsTransactionService.applyCancelResult(response); | ||
|
|
||
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("payment_key") | ||
| .orElseThrow(); | ||
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.CANCELED); | ||
| TosspaymentsPaymentResult updatedResult = tosspaymentsPaymentResultRepository | ||
| .findByTosspaymentsPaymentId(updatedPayment.getId()).orElseThrow(); | ||
| assertThat(updatedResult.getCancelReason()).isEqualTo("단순 변심"); | ||
| assertThat(updatedResult.getCanceledAt()).isEqualTo(canceledAt); | ||
| assertThat(updatedResult.getCancelStatus()).isEqualTo("DONE"); | ||
| } | ||
|
|
||
| @Test | ||
| void 취소_결과에_취소_정보가_없으면_결제_상태만_변경한다() { | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("payment_key") | ||
| .build(); | ||
| payment.updateState(PaymentState.COMPLETED); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
|
|
||
| TosspaymentsPaymentResult result = TosspaymentsPaymentResultFixtureBuilder.builder() | ||
| .tosspaymentsPayment(payment) | ||
| .build(); | ||
| tosspaymentsPaymentResultRepository.save(result); | ||
|
|
||
| TosspaymentsPaymentResponse response = new TosspaymentsPaymentResponse( | ||
| "payment_key", PaymentType.NORMAL, "order_id", "주문명", | ||
| "tvivarepublica", "KRW", "카드", 10000L, 10000L, | ||
| TosspaymentsStatus.CANCELED, LocalDateTime.now(), LocalDateTime.now(), | ||
| "tx_key", 9091L, 909L, 0L, 0L, true, | ||
| "{}", null, null, null, "KR", null, null | ||
| ); | ||
|
|
||
| tosspaymentsTransactionService.applyCancelResult(response); | ||
|
|
||
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("payment_key") | ||
| .orElseThrow(); | ||
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.CANCELED); | ||
| TosspaymentsPaymentResult updatedResult = tosspaymentsPaymentResultRepository | ||
| .findByTosspaymentsPaymentId(updatedPayment.getId()).orElseThrow(); | ||
| assertThat(updatedResult.getCancelReason()).isNull(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
두 테스트 메서드의 공통 setup 로직 추출 고려
두 테스트 모두 TosspaymentsPayment 생성 → COMPLETED 상태 업데이트 → 저장 → TosspaymentsPaymentResult 생성 → 저장의 동일한 패턴을 반복하고 있습니다. 헬퍼 메서드로 추출하면 테스트 시나리오가 추가될 때 유지보수가 용이합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionServiceTest.java`
around lines 33 - 100, Both test methods duplicate the setup of a completed
TosspaymentsPayment and its TosspaymentsPaymentResult; extract that into a
private helper (e.g., createCompletedPaymentWithResult or setUpCompletedPayment)
inside TosspaymentsTransactionServiceTest to return the saved
TosspaymentsPayment (or its id) and associated TosspaymentsPaymentResult. The
helper should call TosspaymentsPaymentFixtureBuilder.builder(), set paymentKey,
call payment.updateState(PaymentState.COMPLETED), persist with
tosspaymentsPaymentRepository.save(payment), create and persist the
TosspaymentsPaymentResult via TosspaymentsPaymentResultFixtureBuilder and
tosspaymentsPaymentResultRepository.save(result); then replace the duplicated
blocks in both tests with calls to this helper.
Comment on lines
+33
to
+69
| @Test | ||
| void 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() { | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("payment_key") | ||
| .build(); | ||
| payment.updateState(PaymentState.COMPLETED); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
|
|
||
| TosspaymentsPaymentResult result = TosspaymentsPaymentResultFixtureBuilder.builder() | ||
| .tosspaymentsPayment(payment) | ||
| .build(); | ||
| tosspaymentsPaymentResultRepository.save(result); | ||
|
|
||
| LocalDateTime canceledAt = LocalDateTime.of(2025, 1, 1, 12, 0); | ||
| TosspaymentsCancel cancel = new TosspaymentsCancel( | ||
| "cancel_tx_key", "단순 변심", 0L, | ||
| canceledAt, 0L, null, 10000L, 0L, 10000L, "DONE", null | ||
| ); | ||
| TosspaymentsPaymentResponse response = new TosspaymentsPaymentResponse( | ||
| "payment_key", PaymentType.NORMAL, "order_id", "주문명", | ||
| "tvivarepublica", "KRW", "카드", 10000L, 10000L, | ||
| TosspaymentsStatus.CANCELED, LocalDateTime.now(), LocalDateTime.now(), | ||
| "cancel_tx_key", 9091L, 909L, 0L, 0L, true, | ||
| "{}", null, null, null, "KR", null, List.of(cancel) | ||
| ); | ||
|
|
||
| tosspaymentsTransactionService.applyCancelResult(response); | ||
|
|
||
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("payment_key") | ||
| .orElseThrow(); | ||
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.CANCELED); | ||
| TosspaymentsPaymentResult updatedResult = tosspaymentsPaymentResultRepository | ||
| .findByTosspaymentsPaymentId(updatedPayment.getId()).orElseThrow(); | ||
| assertThat(updatedResult.getCancelReason()).isEqualTo("단순 변심"); | ||
| assertThat(updatedResult.getCanceledAt()).isEqualTo(canceledAt); | ||
| assertThat(updatedResult.getCancelStatus()).isEqualTo("DONE"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
취소 정보 업데이트 시 lastTransactionKey 검증 누락
response에 "cancel_tx_key"를 lastTransactionKey로 전달하고 있지만(Line 55), 취소 결과 적용 후 updatedResult의 lastTransactionKey 값을 검증하지 않습니다. 취소 흐름에서 lastTransactionKey가 올바르게 업데이트되는지 확인하는 assertion을 추가하는 것이 좋습니다.
💡 검증 추가 제안
assertThat(updatedResult.getCancelReason()).isEqualTo("단순 변심");
assertThat(updatedResult.getCanceledAt()).isEqualTo(canceledAt);
assertThat(updatedResult.getCancelStatus()).isEqualTo("DONE");
+ assertThat(updatedResult.getLastTransactionKey()).isEqualTo("cancel_tx_key");📝 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
| @Test | |
| void 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() { | |
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | |
| .paymentKey("payment_key") | |
| .build(); | |
| payment.updateState(PaymentState.COMPLETED); | |
| tosspaymentsPaymentRepository.save(payment); | |
| TosspaymentsPaymentResult result = TosspaymentsPaymentResultFixtureBuilder.builder() | |
| .tosspaymentsPayment(payment) | |
| .build(); | |
| tosspaymentsPaymentResultRepository.save(result); | |
| LocalDateTime canceledAt = LocalDateTime.of(2025, 1, 1, 12, 0); | |
| TosspaymentsCancel cancel = new TosspaymentsCancel( | |
| "cancel_tx_key", "단순 변심", 0L, | |
| canceledAt, 0L, null, 10000L, 0L, 10000L, "DONE", null | |
| ); | |
| TosspaymentsPaymentResponse response = new TosspaymentsPaymentResponse( | |
| "payment_key", PaymentType.NORMAL, "order_id", "주문명", | |
| "tvivarepublica", "KRW", "카드", 10000L, 10000L, | |
| TosspaymentsStatus.CANCELED, LocalDateTime.now(), LocalDateTime.now(), | |
| "cancel_tx_key", 9091L, 909L, 0L, 0L, true, | |
| "{}", null, null, null, "KR", null, List.of(cancel) | |
| ); | |
| tosspaymentsTransactionService.applyCancelResult(response); | |
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("payment_key") | |
| .orElseThrow(); | |
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.CANCELED); | |
| TosspaymentsPaymentResult updatedResult = tosspaymentsPaymentResultRepository | |
| .findByTosspaymentsPaymentId(updatedPayment.getId()).orElseThrow(); | |
| assertThat(updatedResult.getCancelReason()).isEqualTo("단순 변심"); | |
| assertThat(updatedResult.getCanceledAt()).isEqualTo(canceledAt); | |
| assertThat(updatedResult.getCancelStatus()).isEqualTo("DONE"); | |
| } | |
| `@Test` | |
| void 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() { | |
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | |
| .paymentKey("payment_key") | |
| .build(); | |
| payment.updateState(PaymentState.COMPLETED); | |
| tosspaymentsPaymentRepository.save(payment); | |
| TosspaymentsPaymentResult result = TosspaymentsPaymentResultFixtureBuilder.builder() | |
| .tosspaymentsPayment(payment) | |
| .build(); | |
| tosspaymentsPaymentResultRepository.save(result); | |
| LocalDateTime canceledAt = LocalDateTime.of(2025, 1, 1, 12, 0); | |
| TosspaymentsCancel cancel = new TosspaymentsCancel( | |
| "cancel_tx_key", "단순 변심", 0L, | |
| canceledAt, 0L, null, 10000L, 0L, 10000L, "DONE", null | |
| ); | |
| TosspaymentsPaymentResponse response = new TosspaymentsPaymentResponse( | |
| "payment_key", PaymentType.NORMAL, "order_id", "주문명", | |
| "tvivarepublica", "KRW", "카드", 10000L, 10000L, | |
| TosspaymentsStatus.CANCELED, LocalDateTime.now(), LocalDateTime.now(), | |
| "cancel_tx_key", 9091L, 909L, 0L, 0L, true, | |
| "{}", null, null, null, "KR", null, List.of(cancel) | |
| ); | |
| tosspaymentsTransactionService.applyCancelResult(response); | |
| TosspaymentsPayment updatedPayment = tosspaymentsPaymentRepository.findByPaymentKey("payment_key") | |
| .orElseThrow(); | |
| assertThat(updatedPayment.getState()).isEqualTo(PaymentState.CANCELED); | |
| TosspaymentsPaymentResult updatedResult = tosspaymentsPaymentResultRepository | |
| .findByTosspaymentsPaymentId(updatedPayment.getId()).orElseThrow(); | |
| assertThat(updatedResult.getCancelReason()).isEqualTo("단순 변심"); | |
| assertThat(updatedResult.getCanceledAt()).isEqualTo(canceledAt); | |
| assertThat(updatedResult.getCancelStatus()).isEqualTo("DONE"); | |
| assertThat(updatedResult.getLastTransactionKey()).isEqualTo("cancel_tx_key"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionServiceTest.java`
around lines 33 - 69, The test 취소_결과에_취소_정보가_있으면_결제_결과에_취소_정보를_업데이트한다() omits
asserting that lastTransactionKey was updated from the response; after calling
tosspaymentsTransactionService.applyCancelResult(response) and loading
updatedResult (via findByTosspaymentsPaymentId), add an assertion that
updatedResult.getLastTransactionKey() equals the "cancel_tx_key" provided in the
TosspaymentsPaymentResponse so the cancel flow (applyCancelResult) properly
updates lastTransactionKey.
Comment on lines
+42
to
+48
| import org.springframework.test.context.bean.override.mockito.MockitoBean; | ||
| import org.springframework.restdocs.payload.JsonFieldType; | ||
|
|
||
| class TokenControllerTest extends BaseControllerTest { | ||
|
|
||
| @MockitoBean | ||
| private PaymentFacadeService paymentFacadeService; |
There was a problem hiding this comment.
컨트롤러 테스트에서 서비스 모킹은 가이드 위반입니다.
현재 @MockitoBean PaymentFacadeService로 컨트롤러 테스트가 단위 테스트 형태가 됩니다. 서비스는 실제 빈으로 두고, 외부 결제 클라이언트(예: TosspaymentsClient)만 목킹해 MockMvc 통합 테스트 구조를 유지해주세요. 이때 stubbing도 클라이언트 응답 기준으로 이동이 필요합니다.
As per coding guidelines, 'Use controller tests with MockMvc and real beans for integration testing and REST Docs generation'.
Also applies to: 109-114, 552-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/com/samhap/kokomen/token/controller/TokenControllerTest.java`
around lines 42 - 48, The test currently mocks the service layer (remove the
`@MockitoBean` on PaymentFacadeService in TokenControllerTest) which violates the
integration-test-with-MockMvc guideline; instead keep PaymentFacadeService as
the real Spring bean and mock only the external payment client (e.g.,
TosspaymentsClient), moving any stubbing that targets PaymentFacadeService to
stub the TosspaymentsClient responses so MockMvc exercises the real
service+controller wiring and REST Docs generation still works; apply the same
change to the other occurrences referenced (the other `@MockitoBean` usages around
the file).
Comment on lines
+25
to
+26
| tosspayments: | ||
| widget-secret-key: test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6 |
There was a problem hiding this comment.
테스트 리소스에 시크릿 키 하드코딩은 피해야 합니다.
Line 26의 widget-secret-key가 실제 키 형태로 보입니다. 테스트 환경이라도 레포에 시크릿을 직접 넣으면 노출/회전 리스크가 커집니다. 환경 변수로 주입하고 기본값은 더미로 두는 방식을 권장합니다.
🔧 제안 수정안
-tosspayments:
- widget-secret-key: test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6
+tosspayments:
+ widget-secret-key: ${TOSSPAYMENTS_WIDGET_SECRET_KEY:test_dummy_key}📝 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
| tosspayments: | |
| widget-secret-key: test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6 | |
| tosspayments: | |
| widget-secret-key: ${TOSSPAYMENTS_WIDGET_SECRET_KEY:test_dummy_key} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/application.yml` around lines 25 - 26, 현재 테스트 리소스에 하드코딩된
secret 값(widget-secret-key)이 포함되어 있으니 이를 제거하고 환경변수 주입으로 변경하세요: application.yml의
widget-secret-key 항목을 하드코딩된 값 대신 플레이스홀더로 바꿔 예: widget-secret-key:
${WIDGET_SECRET_KEY:dummy_widget_secret} 처럼 기본 더미 값을 제공하고, 테스트/CI에서는 필요한 경우
WIDGET_SECRET_KEY를 설정하도록 하며 실제 시크릿은 레포에 커밋하지 않도록 처리합니다.
Comment on lines
+16
to
+22
| @Getter | ||
| @Component | ||
| public class TossPaymentsClientBuilder { | ||
|
|
||
| private static final String TOSSPAYMENTS_API_URL = "https://api.tosspayments.com"; | ||
|
|
||
| private final RestClient.Builder tossPaymentsClientBuilder; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
RestClient.Builder를 getter로 외부에 노출하면 공유 가변 상태 문제가 발생할 수 있습니다.
@Getter로 tossPaymentsClientBuilder를 노출하면 외부에서 빌더의 설정을 변경할 수 있고, 이는 다른 곳에서 빌드하는 RestClient에 의도치 않은 영향을 줄 수 있습니다. RestClient를 직접 빌드하여 불변 객체를 노출하거나, 빌더를 clone하여 반환하는 방식을 고려해 주세요.
♻️ RestClient를 직접 노출하는 방식 예시
-@Getter
`@Component`
-public class TossPaymentsClientBuilder {
+public class TosspaymentsClientBuilder {
- private final RestClient.Builder tossPaymentsClientBuilder;
+ private final RestClient tossPaymentsRestClient;
- public TossPaymentsClientBuilder(...) {
+ public TosspaymentsClientBuilder(...) {
...
- this.tossPaymentsClientBuilder = builder
+ this.tossPaymentsRestClient = builder
.requestFactory(requestFactory)
.baseUrl(TOSSPAYMENTS_API_URL)
.defaultHeader("Authorization", "Basic " + encodedSecretKey)
.defaultHeader("Content-Type", "application/json")
.messageConverters(converters -> {
converters.removeIf(MappingJackson2HttpMessageConverter.class::isInstance);
converters.add(new MappingJackson2HttpMessageConverter(createObjectMapper()));
- });
+ })
+ .build();
}
+
+ public RestClient getRestClient() {
+ return tossPaymentsRestClient;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java`
around lines 16 - 22, 현재 TossPaymentsClientBuilder 클래스는 `@Getter로`
tossPaymentsClientBuilder(RestClient.Builder)를 외부에 노출하고 있어 공유 가변 상태 이슈가 발생합니다;
`@Getter를` 제거하고 tossPaymentsClientBuilder를 private으로 유지한 후 외부에 RestClient 인스턴스만
제공하도록 변경하세요 — 예: TossPaymentsClientBuilder에 public RestClient
createTossPaymentsClient() 혹은 getTossPaymentsClient() 메서드를 추가해 내부의
tossPaymentsClientBuilder로부터 새로운 RestClient를 매번 빌드해 반환하거나(빌더가 재사용될 수 있게 매 호출마다
새로운 Builder를 생성) 빌더를 안전하게 복제한 뒤 반환하도록 구현하고, TOSSPAYMENTS_API_URL과 관련된 설정은 이 생성
메서드 내부에서만 적용해 불변 객체만 외부로 노출되게 하세요 (참조: TossPaymentsClientBuilder,
tossPaymentsClientBuilder, RestClient.Builder, TOSSPAYMENTS_API_URL).
Comment on lines
+23
to
+34
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime requestedAt, | ||
| @JsonDeserialize(using = TossDateTimeDeserializer.class) | ||
| LocalDateTime approvedAt, | ||
| String lastTransactionKey, | ||
| Long suppliedAmount, | ||
| Long vat, | ||
| Long taxFreeAmount, | ||
| Long taxExemptionAmount, | ||
| boolean isPartialCancelable, | ||
| @JsonDeserialize(using = ObjectToStringDeserializer.class) | ||
| String metadata, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check PaymentResponse usage patterns
rg -n "PaymentResponse" --type=java -A 3 -B 3 | head -100Repository: samhap-soft/kokomen-backend
Length of output: 12567
🏁 Script executed:
# Search for direct deserialization of PaymentResponse
rg -n "readValue.*PaymentResponse|objectMapper.*PaymentResponse|mapper.*PaymentResponse" --type=javaRepository: samhap-soft/kokomen-backend
Length of output: 53
🏁 Script executed:
# Check if PaymentResponse is used in controller responses
rg -n "ResponseEntity.*PaymentResponse|@ResponseBody.*PaymentResponse" --type=java -A 2Repository: samhap-soft/kokomen-backend
Length of output: 53
🏁 Script executed:
# Look at the actual PaymentResponse class definition
find . -name "PaymentResponse.java" -type f | head -5Repository: samhap-soft/kokomen-backend
Length of output: 146
🏁 Script executed:
cat -n ./src/main/java/com/samhap/kokomen/payment/service/dto/PaymentResponse.javaRepository: samhap-soft/kokomen-backend
Length of output: 3461
@JsonDeserialize 어노테이션을 제거하세요.
PaymentResponse는 이미 역직렬화된 TosspaymentsPaymentResponse로부터 from() 팩토리 메서드로만 생성됩니다. 코드베이스 어디에서도 PaymentResponse가 직접 JSON으로부터 역직렬화되지 않으므로, requestedAt, approvedAt, metadata의 @JsonDeserialize 어노테이션은 불필요한 데드 코드입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/service/dto/PaymentResponse.java`
around lines 23 - 34, Remove the unnecessary Jackson deserialization annotations
from PaymentResponse: delete `@JsonDeserialize`(using =
TossDateTimeDeserializer.class) on requestedAt and approvedAt and
`@JsonDeserialize`(using = ObjectToStringDeserializer.class) on metadata;
PaymentResponse is only created via the from() factory that accepts a
TosspaymentsPaymentResponse, so keep the fields as plain LocalDateTime/ String
and ensure the from() method remains the single construction path.
Comment on lines
+39
to
+77
| public TosspaymentsPaymentResultFixtureBuilder tosspaymentsPayment(TosspaymentsPayment tosspaymentsPayment) { | ||
| this.tosspaymentsPayment = tosspaymentsPayment; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder type(PaymentType type) { | ||
| this.type = type; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder method(String method) { | ||
| this.method = method; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder tosspaymentsStatus(TosspaymentsStatus status) { | ||
| this.tosspaymentsStatus = status; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder approvedAt(LocalDateTime approvedAt) { | ||
| this.approvedAt = approvedAt; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder receiptUrl(String receiptUrl) { | ||
| this.receiptUrl = receiptUrl; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder failureCode(String failureCode) { | ||
| this.failureCode = failureCode; | ||
| return this; | ||
| } | ||
|
|
||
| public TosspaymentsPaymentResultFixtureBuilder failureMessage(String failureMessage) { | ||
| this.failureMessage = failureMessage; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cat -n src/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.javaRepository: samhap-soft/kokomen-backend
Length of output: 4681
🏁 Script executed:
rg -n "TosspaymentsPaymentResultFixtureBuilder" --type=java -B 2 -A 15Repository: samhap-soft/kokomen-backend
Length of output: 23497
🏁 Script executed:
rg -n "\.totalAmount\(|\.balanceAmount\(|\.isPartialCancelable\(" --type=javaRepository: samhap-soft/kokomen-backend
Length of output: 3486
현재 테스트에서는 추가 setter가 필요하지 않습니다.
실제 테스트 코드 사용 현황을 보면, TosspaymentsPaymentResultFixtureBuilder는 .tosspaymentsPayment(payment).build() 패턴으로만 사용되고 있으며, totalAmount, balanceAmount, isPartialCancelable 등의 메서드 호출은 없습니다. 이들 필드는 이미 build() 메서드의 기본값으로 충분히 처리되고 있습니다.
향후 테스트 시나리오 확장 시 필요하다면 해당 setter들을 추가하는 것이 좋겠지만, 현재로서는 선택사항입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java`
around lines 39 - 77, The builder has extra setter methods that aren’t used by
current tests; in TosspaymentsPaymentResultFixtureBuilder remove the unnecessary
setters (type, method, tosspaymentsStatus, approvedAt, receiptUrl, failureCode,
failureMessage) and keep only tosspaymentsPayment(...) (and build()), relying on
the build() method’s existing default values for totalAmount, balanceAmount,
isPartialCancelable, etc.; ensure build() still composes the fixture correctly
after removing these methods.
.../java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java (2)
29-30:⚠️ Potential issue | 🟠 Major응답 객체 전체 로깅 시 민감 정보 노출 위험
응답에도 결제 수단, 카드 정보 등 민감한 데이터가 포함될 수 있습니다. 요청 인자와 동일하게
DEBUG레벨 사용 또는 선택적 필드만 로깅하는 방안을 고려해 주세요.♻️ DEBUG 레벨로 변경 제안
- log.info("[토스페이먼츠 API 응답] {} - {}ms - response: {}", - methodName, stopWatch.getTotalTimeMillis(), result); + log.info("[토스페이먼츠 API 응답] {} - {}ms", methodName, stopWatch.getTotalTimeMillis()); + log.debug("[토스페이먼츠 API 응답 상세] {} - response: {}", methodName, result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java` around lines 29 - 30, The current logging in TosspaymentsLoggingAspect logs the full response at INFO level which may expose sensitive payment data; change the log to DEBUG (or conditionally log only non-sensitive fields) in the logging statement that uses methodName, stopWatch.getTotalTimeMillis(), and result so full response output is not emitted at INFO; alternatively implement a sanitizer that masks sensitive fields on the result before logging and call that sanitizer from the same place where the log currently occurs.
22-22:⚠️ Potential issue | 🟠 Major요청 인자를 INFO 레벨로 전체 로깅 시 민감 정보 노출 위험
args에는 결제 키, 금액 등 민감한 데이터가 포함될 수 있습니다. 전체 인자를INFO레벨로 로깅하면 로그 수집 시스템에 PII/결제 정보가 유출될 수 있습니다.DEBUG레벨로 변경하거나, 민감 필드를 마스킹하는 것을 권장합니다.♻️ DEBUG 레벨로 변경 제안
- log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args); + log.debug("[토스페이먼츠 API 요청] {} - args: {}", methodName, args);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java` at line 22, The current logging in TosspaymentsLoggingAspect logs all request args at INFO level (log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName, args)), risking exposure of sensitive payment/PII data; change this to a safer approach by either lowering to DEBUG (use log.debug(...) for methodName and args) or sanitize/mask sensitive fields in the args before logging (implement a helper in TosspaymentsLoggingAspect to redact keys like cardNumber, paymentKey, amount, etc., and log the sanitized object), and ensure the chosen approach is used consistently wherever log.info("[토스페이먼츠 API 요청]...") appears.src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java (2)
19-19: 🧹 Nitpick | 🔵 Trivial클래스명이 다른 Tosspayments 관련 클래스들과 불일치합니다.
TosspaymentsClient,TosspaymentsPayment,TosspaymentsPaymentResult등은Tosspayments(소문자 p)를 사용하지만, 이 클래스만TossPayments(대문자 P)를 사용합니다. 프로젝트 일관성을 위해TosspaymentsClientBuilder로 변경을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java` at line 19, Rename the class TossPaymentsClientBuilder to TosspaymentsClientBuilder to match existing naming (TosspaymentsClient, TosspaymentsPayment, TosspaymentsPaymentResult); update the public class declaration and any constructors, static factory methods, imports, usages, and tests that reference TossPaymentsClientBuilder to the new TosspaymentsClientBuilder identifier so all references compile and follow the same capitalization convention.
17-23: 🛠️ Refactor suggestion | 🟠 Major
@Getter를 통한RestClient.Builder노출은 공유 가변 상태 문제를 야기합니다.
TosspaymentsClient에서getTossPaymentsClientBuilder().build()를 호출하여 사용하고 있으나, 빌더가 외부로 노출되면 다른 곳에서 설정을 변경할 수 있어 의도치 않은 사이드 이펙트가 발생할 수 있습니다.RestClient를 직접 빌드하여 불변 객체로 노출하는 것을 권장합니다.♻️ RestClient를 직접 노출하는 방식으로 리팩토링 제안
-@Getter `@Component` public class TossPaymentsClientBuilder { private static final String TOSSPAYMENTS_API_URL = "https://api.tosspayments.com"; - private final RestClient.Builder tossPaymentsClientBuilder; + private final RestClient tossPaymentsRestClient; public TossPaymentsClientBuilder( RestClient.Builder builder, `@Value`("${tosspayments.widget-secret-key}") String tossPaymentsWidgetSecretKey) { // ... 기존 설정 코드 ... - this.tossPaymentsClientBuilder = builder + this.tossPaymentsRestClient = builder .requestFactory(requestFactory) .baseUrl(TOSSPAYMENTS_API_URL) .defaultHeader("Authorization", "Basic " + encodedSecretKey) .defaultHeader("Content-Type", "application/json") .messageConverters(converters -> { converters.removeIf(MappingJackson2HttpMessageConverter.class::isInstance); converters.add(new MappingJackson2HttpMessageConverter(createObjectMapper())); - }); + }) + .build(); } + + public RestClient getTossPaymentsRestClient() { + return tossPaymentsRestClient; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java` around lines 17 - 23, TossPaymentsClientBuilder currently exposes a mutable RestClient.Builder via the `@Getter` on tossPaymentsClientBuilder causing shared mutable state; instead, build the RestClient once inside TossPaymentsClientBuilder (using TOSSPAYMENTS_API_URL) and expose an immutable RestClient instance (e.g., tossPaymentsClient) so callers (like TossPaymentsClient) no longer call getTossPaymentsClientBuilder().build(); remove the `@Getter` on tossPaymentsClientBuilder and replace any usage of getTossPaymentsClientBuilder() in TossPaymentsClient with the new RestClient accessor.src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.java (2)
39-39: 🧹 Nitpick | 🔵 Trivial
java.util.ListFQN 대신 import 사용다른 타입들은 import를 사용하고 있지만
cancels필드만 FQN(fully qualified name)을 사용하여 일관성이 떨어집니다.♻️ 수정 제안
import 추가:
import java.util.List;필드 선언 변경:
- java.util.List<TosspaymentsCancel> cancels + List<TosspaymentsCancel> cancels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.java` at line 39, TosspaymentsPaymentResponse의 cancels 필드가 java.util.List로 FQN을 사용해 다른 타입과 일관성이 떨어집니다; 파일 상단에 import java.util.List;를 추가하고 클래스의 필드 선언에서 "java.util.List<TosspaymentsCancel> cancels"를 "List<TosspaymentsCancel> cancels"로 변경해 주세요.
60-66: 🧹 Nitpick | 🔵 Trivial반복되는 null 체크 간소화
easyPay(),receipt(),failure()메서드가 여러 번 호출되고 있습니다. 로컬 변수로 추출하면 가독성과 효율성이 개선됩니다.♻️ 로컬 변수 추출 예시
public TosspaymentsPaymentResult toTosspaymentsPaymentResult(TosspaymentsPayment tosspaymentsPayment) { + var receipt = this.receipt(); + var easyPay = this.easyPay(); + var failure = this.failure(); return TosspaymentsPaymentResult.builder() // ... 중략 ... - .receiptUrl(this.receipt() != null ? this.receipt().url() : null) - .easyPayProvider(this.easyPay() != null ? this.easyPay().provider() : null) - .easyPayAmount(this.easyPay() != null ? this.easyPay().amount() : null) - .easyPayDiscountAmount(this.easyPay() != null ? this.easyPay().discountAmount() : null) + .receiptUrl(receipt != null ? receipt.url() : null) + .easyPayProvider(easyPay != null ? easyPay.provider() : null) + .easyPayAmount(easyPay != null ? easyPay.amount() : null) + .easyPayDiscountAmount(easyPay != null ? easyPay.discountAmount() : null) .country(this.country) - .failureCode(this.failure() != null ? this.failure().code() : null) - .failureMessage(this.failure() != null ? this.failure().message() : null) + .failureCode(failure != null ? failure.code() : null) + .failureMessage(failure != null ? failure.message() : null) .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.java` around lines 60 - 66, The builder chain in TosspaymentsPaymentResponse repeatedly calls receipt(), easyPay(), and failure(); extract these into local variables (e.g., var receipt = this.receipt(); var easyPay = this.easyPay(); var failure = this.failure()) at the start of the method that builds the DTO, then replace repeated calls with receipt, easyPay, and failure when setting receiptUrl, easyPayProvider, easyPayAmount, easyPayDiscountAmount, failureCode, and failureMessage to simplify null-checks and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.java`:
- Around line 10-12: Replace the direct JDK exceptions in
TosspaymentsPaymentCancelRequest (the null/blank checks using
Objects.requireNonNull and throwing IllegalArgumentException) with the project's
BadRequestException to keep error responses consistent; specifically, validate
cancelReason and when null or blank throw new BadRequestException with the same
descriptive messages (e.g., "cancelReason은 필수입니다." and "cancelReason은 비어있을 수
없습니다.") so callers receive the unified custom exception type.
In
`@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java`:
- Around line 28-30: The connect and read timeout values are hardcoded where
SimpleClientHttpRequestFactory is created in TossPaymentsClientBuilder; change
this to read configurable properties via `@Value-injected` fields (e.g., int
connectTimeoutMs and int readTimeoutMs) on the TossPaymentsClientBuilder class,
use those injected values when calling requestFactory.setConnectTimeout(...) and
requestFactory.setReadTimeout(...), and provide sensible defaults in application
properties (or via `@Value` default expressions) so environments can override
them.
In
`@src/main/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionService.java`:
- Around line 30-44: The code currently sets
payment.updateState(PaymentState.CANCELED) before verifying response.cancels(),
which can leave the Payment in CANCELED without cancel details; change the flow
in TosspaymentsTransactionService so you first validate that response.cancels()
is non-null and non-empty (and that the last TosspaymentsCancel has required
fields) before calling payment.updateState(PaymentState.CANCELED) and
result.updateCancelInfo(...); if validation fails throw a BadRequestException
(or another appropriate custom exception) so the transaction is rolled back;
keep references to
tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(...) and
result.updateCancelInfo(...) when applying the update after validation.
In `@src/main/java/com/samhap/kokomen/token/dto/TokenPurchaseRequest.java`:
- Around line 29-42: The DTO TokenPurchaseRequest currently takes an
ObjectMapper and performs JSON serialization inside toPaymentConfirmRequest,
coupling the DTO to infrastructure; move the metadata creation and serialization
out of TokenPurchaseRequest into the service or a mapper class (e.g.,
createPurchaseMetadata and serialize it in the service), so
toPaymentConfirmRequest only constructs/returns a PurchaseMetadata (or raw
fields) and the service uses ObjectMapper to produce metadataJson; update
references to TokenPurchaseRequest.toPaymentConfirmRequest, PurchaseMetadata,
metadataJson and remove ObjectMapper dependency from the DTO.
In `@src/main/resources/db/migration/V40__alter_canceled_at_to_datetime6.sql`:
- Line 1: The ALTER TABLE statement that modifies the canceled_at column using
MODIFY COLUMN removed the DEFAULT NULL attribute; update the ALTER TABLE
tosspayments_payment_result MODIFY COLUMN canceled_at to explicitly include
DEFAULT NULL so the canceled_at DATETIME(6) column preserves its NULL default
(i.e., change the MODIFY COLUMN for canceled_at to specify DEFAULT NULL).
In
`@src/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java`:
- Around line 39-42: The TosspaymentsPaymentResultFixtureBuilder currently
allows creating a TosspaymentsPaymentResult without the required
tosspaymentsPayment (which is declared nullable = false); update the builder's
build() method in TosspaymentsPaymentResultFixtureBuilder to validate that
this.tosspaymentsPayment is not null and throw a clear IllegalStateException (or
similar) if it is missing, so tests fail fast and indicate the missing required
field when building TosspaymentsPaymentResult.
---
Duplicate comments:
In `@src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.java`:
- Around line 29-30: The current logging in TosspaymentsLoggingAspect logs the
full response at INFO level which may expose sensitive payment data; change the
log to DEBUG (or conditionally log only non-sensitive fields) in the logging
statement that uses methodName, stopWatch.getTotalTimeMillis(), and result so
full response output is not emitted at INFO; alternatively implement a sanitizer
that masks sensitive fields on the result before logging and call that sanitizer
from the same place where the log currently occurs.
- Line 22: The current logging in TosspaymentsLoggingAspect logs all request
args at INFO level (log.info("[토스페이먼츠 API 요청] {} - args: {}", methodName,
args)), risking exposure of sensitive payment/PII data; change this to a safer
approach by either lowering to DEBUG (use log.debug(...) for methodName and
args) or sanitize/mask sensitive fields in the args before logging (implement a
helper in TosspaymentsLoggingAspect to redact keys like cardNumber, paymentKey,
amount, etc., and log the sanitized object), and ensure the chosen approach is
used consistently wherever log.info("[토스페이먼츠 API 요청]...") appears.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.java`:
- Line 39: TosspaymentsPaymentResponse의 cancels 필드가 java.util.List로 FQN을 사용해 다른
타입과 일관성이 떨어집니다; 파일 상단에 import java.util.List;를 추가하고 클래스의 필드 선언에서
"java.util.List<TosspaymentsCancel> cancels"를 "List<TosspaymentsCancel>
cancels"로 변경해 주세요.
- Around line 60-66: The builder chain in TosspaymentsPaymentResponse repeatedly
calls receipt(), easyPay(), and failure(); extract these into local variables
(e.g., var receipt = this.receipt(); var easyPay = this.easyPay(); var failure =
this.failure()) at the start of the method that builds the DTO, then replace
repeated calls with receipt, easyPay, and failure when setting receiptUrl,
easyPayProvider, easyPayAmount, easyPayDiscountAmount, failureCode, and
failureMessage to simplify null-checks and improve readability.
In
`@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java`:
- Line 19: Rename the class TossPaymentsClientBuilder to
TosspaymentsClientBuilder to match existing naming (TosspaymentsClient,
TosspaymentsPayment, TosspaymentsPaymentResult); update the public class
declaration and any constructors, static factory methods, imports, usages, and
tests that reference TossPaymentsClientBuilder to the new
TosspaymentsClientBuilder identifier so all references compile and follow the
same capitalization convention.
- Around line 17-23: TossPaymentsClientBuilder currently exposes a mutable
RestClient.Builder via the `@Getter` on tossPaymentsClientBuilder causing shared
mutable state; instead, build the RestClient once inside
TossPaymentsClientBuilder (using TOSSPAYMENTS_API_URL) and expose an immutable
RestClient instance (e.g., tossPaymentsClient) so callers (like
TossPaymentsClient) no longer call getTossPaymentsClientBuilder().build();
remove the `@Getter` on tossPaymentsClientBuilder and replace any usage of
getTossPaymentsClientBuilder() in TossPaymentsClient with the new RestClient
accessor.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
src/main/java/com/samhap/kokomen/global/aop/TosspaymentsLoggingAspect.javasrc/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.javasrc/main/java/com/samhap/kokomen/global/exception/NotFoundException.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResult.javasrc/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TossDateTimeDeserializer.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsConfirmRequest.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentResponse.javasrc/main/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionService.javasrc/main/java/com/samhap/kokomen/token/dto/TokenPurchaseRequest.javasrc/main/resources/db/migration/V40__alter_canceled_at_to_datetime6.sqlsrc/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java
src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.java
Outdated
Show resolved
Hide resolved
Comment on lines
+28
to
+30
| SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory(); | ||
| requestFactory.setConnectTimeout(3000); | ||
| requestFactory.setReadTimeout(15000); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
타임아웃 값을 외부 설정으로 분리하는 것을 고려해 주세요.
현재 connect timeout(3000ms)과 read timeout(15000ms)이 하드코딩되어 있습니다. 환경별로 다른 타임아웃이 필요할 수 있으므로, @Value 어노테이션을 통해 설정 파일에서 주입받는 방식을 권장합니다.
♻️ 타임아웃 설정 외부화 예시
public TossPaymentsClientBuilder(
RestClient.Builder builder,
- `@Value`("${tosspayments.widget-secret-key}") String tossPaymentsWidgetSecretKey) {
+ `@Value`("${tosspayments.widget-secret-key}") String tossPaymentsWidgetSecretKey,
+ `@Value`("${tosspayments.connect-timeout:3000}") int connectTimeout,
+ `@Value`("${tosspayments.read-timeout:15000}") int readTimeout) {
SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory();
- requestFactory.setConnectTimeout(3000);
- requestFactory.setReadTimeout(15000);
+ requestFactory.setConnectTimeout(connectTimeout);
+ requestFactory.setReadTimeout(readTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/samhap/kokomen/payment/external/TossPaymentsClientBuilder.java`
around lines 28 - 30, The connect and read timeout values are hardcoded where
SimpleClientHttpRequestFactory is created in TossPaymentsClientBuilder; change
this to read configurable properties via `@Value-injected` fields (e.g., int
connectTimeoutMs and int readTimeoutMs) on the TossPaymentsClientBuilder class,
use those injected values when calling requestFactory.setConnectTimeout(...) and
requestFactory.setReadTimeout(...), and provide sensible defaults in application
properties (or via `@Value` default expressions) so environments can override
them.
Comment on lines
+30
to
+44
| payment.updateState(PaymentState.CANCELED); | ||
|
|
||
| TosspaymentsPaymentResult result = tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(payment.getId()); | ||
|
|
||
| if (response.cancels() != null && !response.cancels().isEmpty()) { | ||
| TosspaymentsCancel tosspaymentsCancel = response.cancels().get(response.cancels().size() - 1); | ||
| result.updateCancelInfo( | ||
| tosspaymentsCancel.cancelReason(), | ||
| tosspaymentsCancel.canceledAt(), | ||
| tosspaymentsCancel.easyPayDiscountAmount(), | ||
| response.lastTransactionKey(), | ||
| tosspaymentsCancel.cancelStatus(), | ||
| response.status() | ||
| ); | ||
| } |
There was a problem hiding this comment.
취소 상세가 비어도 결제 상태를 먼저 CANCELED로 확정해 데이터 불일치가 생길 수 있습니다.
현재는 cancels가 null/empty인 경우에도 Line 30에서 상태가 CANCELED로 변경되고, 취소 상세는 미반영됩니다. 취소 상세 필수 조건을 먼저 검증하고 실패 시 예외로 롤백시키는 방식이 안전합니다.
💡 제안 수정안
`@Transactional`(propagation = Propagation.REQUIRES_NEW)
public void applyCancelResult(TosspaymentsPaymentResponse response) {
TosspaymentsPayment payment = tosspaymentsPaymentService.readByPaymentKey(response.paymentKey());
- payment.updateState(PaymentState.CANCELED);
-
TosspaymentsPaymentResult result = tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(payment.getId());
- if (response.cancels() != null && !response.cancels().isEmpty()) {
- TosspaymentsCancel tosspaymentsCancel = response.cancels().get(response.cancels().size() - 1);
- result.updateCancelInfo(
- tosspaymentsCancel.cancelReason(),
- tosspaymentsCancel.canceledAt(),
- tosspaymentsCancel.easyPayDiscountAmount(),
- response.lastTransactionKey(),
- tosspaymentsCancel.cancelStatus(),
- response.status()
- );
- }
+ if (response.cancels() == null || response.cancels().isEmpty()) {
+ throw new BadRequestException("취소 응답에 cancels 정보가 없습니다.");
+ }
+
+ payment.updateState(PaymentState.CANCELED);
+ TosspaymentsCancel tosspaymentsCancel = response.cancels().get(response.cancels().size() - 1);
+ result.updateCancelInfo(
+ tosspaymentsCancel.cancelReason(),
+ tosspaymentsCancel.canceledAt(),
+ tosspaymentsCancel.easyPayDiscountAmount(),
+ response.lastTransactionKey(),
+ tosspaymentsCancel.cancelStatus(),
+ response.status()
+ );
}As per coding guidelines, 'Use custom exceptions: BadRequestException, UnauthorizedException, ForbiddenException, etc. for exception handling'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/samhap/kokomen/payment/service/TosspaymentsTransactionService.java`
around lines 30 - 44, The code currently sets
payment.updateState(PaymentState.CANCELED) before verifying response.cancels(),
which can leave the Payment in CANCELED without cancel details; change the flow
in TosspaymentsTransactionService so you first validate that response.cancels()
is non-null and non-empty (and that the last TosspaymentsCancel has required
fields) before calling payment.updateState(PaymentState.CANCELED) and
result.updateCancelInfo(...); if validation fails throw a BadRequestException
(or another appropriate custom exception) so the transaction is rolled back;
keep references to
tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(...) and
result.updateCancelInfo(...) when applying the update after validation.
Comment on lines
+29
to
+42
| public ConfirmRequest toPaymentConfirmRequest(Long memberId, ObjectMapper objectMapper) { | ||
| TokenProduct product = readTokenProduct(productName); | ||
| PurchaseMetadata metadata = new PurchaseMetadata( | ||
| productName, | ||
| getTokenCountFromProduct(product), | ||
| product.getUnitPrice() | ||
| ); | ||
|
|
||
| String metadataJson; | ||
| try { | ||
| metadataJson = objectMapper.writeValueAsString(metadata); | ||
| } catch (JsonProcessingException e) { | ||
| throw new InternalServerErrorException("metadata 직렬화 중 오류가 발생했습니다.", e); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DTO가 ObjectMapper 직렬화 책임까지 가지는 구조는 계층 결합도를 높입니다.
TokenPurchaseRequest는 요청 DTO 성격이므로, metadataJson 생성/직렬화는 서비스(또는 매퍼) 계층으로 분리하는 쪽이 유지보수에 더 유리합니다. 현재 구조는 테스트 시 DTO 단위에서 인프라 의존성을 강제합니다.
♻️ 제안 리팩터링
- public ConfirmRequest toPaymentConfirmRequest(Long memberId, ObjectMapper objectMapper) {
+ public ConfirmRequest toPaymentConfirmRequest(Long memberId, String metadataJson) {
TokenProduct product = readTokenProduct(productName);
- PurchaseMetadata metadata = new PurchaseMetadata(
- productName,
- getTokenCountFromProduct(product),
- product.getUnitPrice()
- );
-
- String metadataJson;
- try {
- metadataJson = objectMapper.writeValueAsString(metadata);
- } catch (JsonProcessingException e) {
- throw new InternalServerErrorException("metadata 직렬화 중 오류가 발생했습니다.", e);
- }
return new ConfirmRequest(
paymentKey,
orderId,
price,
orderName,
memberId,
metadataJson,
ServiceType.INTERVIEW
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/token/dto/TokenPurchaseRequest.java` around
lines 29 - 42, The DTO TokenPurchaseRequest currently takes an ObjectMapper and
performs JSON serialization inside toPaymentConfirmRequest, coupling the DTO to
infrastructure; move the metadata creation and serialization out of
TokenPurchaseRequest into the service or a mapper class (e.g.,
createPurchaseMetadata and serialize it in the service), so
toPaymentConfirmRequest only constructs/returns a PurchaseMetadata (or raw
fields) and the service uses ObjectMapper to produce metadataJson; update
references to TokenPurchaseRequest.toPaymentConfirmRequest, PurchaseMetadata,
metadataJson and remove ObjectMapper dependency from the DTO.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE tosspayments_payment_result MODIFY COLUMN canceled_at DATETIME(6); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# canceled_at 컬럼의 기존 정의와 변경 이력 확인
set -euo pipefail
echo "== Flyway 마이그레이션 파일 목록 =="
fd '\.sql$' src/main/resources/db/migration | sort
echo
echo "== tosspayments_payment_result 테이블 관련 마이그레이션 검색 =="
rg -l 'tosspayments_payment_result' src/main/resources/db/migration
echo
echo "== canceled_at 컬럼 관련 SQL 검색 (문맥 포함) =="
rg -B5 -A5 'canceled_at' src/main/resources/db/migration
echo
echo "== V40 마이그레이션 전체 내용 =="
cat src/main/resources/db/migration/V40__alter_canceled_at_to_datetime6.sqlRepository: samhap-soft/kokomen-backend
Length of output: 4640
MODIFY COLUMN 사용 시 DEFAULT NULL 속성이 유실됩니다.
V39에서 canceled_at DATETIME DEFAULT NULL로 정의된 컬럼이 V40에서 타입만 변경되면 MySQL의 MODIFY COLUMN 동작에 의해 DEFAULT NULL 속성이 제거됩니다. 이는 컬럼의 NULL 가능성을 변경하게 되므로 다음과 같이 수정해야 합니다:
ALTER TABLE tosspayments_payment_result MODIFY COLUMN canceled_at DATETIME(6) DEFAULT NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V40__alter_canceled_at_to_datetime6.sql` at
line 1, The ALTER TABLE statement that modifies the canceled_at column using
MODIFY COLUMN removed the DEFAULT NULL attribute; update the ALTER TABLE
tosspayments_payment_result MODIFY COLUMN canceled_at to explicitly include
DEFAULT NULL so the canceled_at DATETIME(6) column preserves its NULL default
(i.e., change the MODIFY COLUMN for canceled_at to specify DEFAULT NULL).
Comment on lines
+39
to
+42
| public TosspaymentsPaymentResultFixtureBuilder tosspaymentsPayment(TosspaymentsPayment tosspaymentsPayment) { | ||
| this.tosspaymentsPayment = tosspaymentsPayment; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
tosspaymentsPayment 필수 필드 확인
TosspaymentsPaymentResult의 tosspaymentsPayment 필드는 nullable = false로 선언되어 있으므로, 빌더 사용 시 반드시 설정해야 합니다. 테스트 코드에서 이를 누락하면 런타임 에러가 발생할 수 있으니 주의하세요.
💡 선택적 개선: build() 메서드에 null 체크 추가
public TosspaymentsPaymentResult build() {
+ if (tosspaymentsPayment == null) {
+ throw new IllegalStateException("tosspaymentsPayment는 필수입니다.");
+ }
return TosspaymentsPaymentResult.builder()
.tosspaymentsPayment(tosspaymentsPayment)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/samhap/kokomen/global/fixture/payment/TosspaymentsPaymentResultFixtureBuilder.java`
around lines 39 - 42, The TosspaymentsPaymentResultFixtureBuilder currently
allows creating a TosspaymentsPaymentResult without the required
tosspaymentsPayment (which is declared nullable = false); update the builder's
build() method in TosspaymentsPaymentResultFixtureBuilder to validate that
this.tosspaymentsPayment is not null and throw a clear IllegalStateException (or
similar) if it is missing, so tests fail fast and indicate the missing required
field when building TosspaymentsPaymentResult.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/samhap/kokomen/global/service/S3Service.java (1)
69-73:⚠️ Potential issue | 🟠 Major
null입력 시BadRequestException으로 매핑되지 않습니다.Line 70에서
cdnUrl이null이면startsWith에서 NPE가 발생해, 의도한 400 계열 예외 처리 경로를 타지 못합니다.수정 제안
private String extractKeyFromCdnUrl(String cdnUrl) { - if (cdnUrl.startsWith(AwsConstant.CLOUD_FRONT_DOMAIN_URL)) { + if (cdnUrl == null || cdnUrl.isBlank()) { + throw new BadRequestException("Invalid CDN URL"); + } + if (cdnUrl.startsWith(AwsConstant.CLOUD_FRONT_DOMAIN_URL)) { return cdnUrl.substring(AwsConstant.CLOUD_FRONT_DOMAIN_URL.length()); } - throw new BadRequestException("Invalid CDN URL: " + cdnUrl); + throw new BadRequestException("Invalid CDN URL"); }As per coding guidelines "Use custom exceptions: BadRequestException, UnauthorizedException, ForbiddenException, etc. for exception handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/global/service/S3Service.java` around lines 69 - 73, In extractKeyFromCdnUrl, guard against null before calling startsWith so a BadRequestException is thrown for null or invalid cdnUrl values; update the method (reference: extractKeyFromCdnUrl and AwsConstant.CLOUD_FRONT_DOMAIN_URL) to first check cdnUrl for null/empty and throw new BadRequestException("Invalid CDN URL: " + cdnUrl) when it fails, otherwise perform the substring operation so no NPE occurs.src/main/java/com/samhap/kokomen/recruit/schedular/service/RecruitmentApiClient.java (1)
110-123:⚠️ Potential issue | 🟠 Major
BadRequestException이 외부 API 응답 검증에 의미적으로 적합하지 않습니다.
BadRequestException은 HTTP 400을 의미하며, 클라이언트(사용자)가 잘못된 요청을 보냈을 때 사용해야 합니다. 그러나 여기서 검증하는 것은 외부 API(zighang.com)의 응답이 null인 경우로, 사용자의 잘못이 아닙니다.현재는
fetchPageSafely()에서 예외를 catch하여 null로 변환하지만, 향후 코드 변경 시 잘못된 HTTP 상태가 사용자에게 전달될 수 있습니다. 외부 API 오류에는InternalServerErrorException이나 별도의 커스텀 예외(예:ExternalApiException)를 사용하는 것이 더 적절합니다.🔧 제안하는 수정 방향
Option 1:
IllegalStateException유지 (catch에서 처리되므로)private void validateResponse(ApiResponse<PagedData<RecruitmentDto>> response, int page) { if (response == null) { - throw new BadRequestException("API 응답이 null입니다 (페이지: " + page + ")"); + throw new IllegalStateException("API 응답이 null입니다 (페이지: " + page + ")"); } // ... 나머지 동일 }Option 2: 외부 API 전용 커스텀 예외 생성
public class ExternalApiException extends KokomenException { public ExternalApiException(String message) { super(message, HttpStatus.SERVICE_UNAVAILABLE); // 503 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/recruit/schedular/service/RecruitmentApiClient.java` around lines 110 - 123, The validateResponse method currently throws BadRequestException for null/invalid external API responses which misuses HTTP 400; change validateResponse to throw a more appropriate server-side exception (e.g., create and throw ExternalApiException or IllegalStateException) so external API failures are represented as 5xx; update the code to define ExternalApiException (extending KokomenException with a 503/Service Unavailable status) and replace BadRequestException throws in validateResponse, and ensure fetchPageSafely continues to catch and handle that exception as before.src/main/java/com/samhap/kokomen/auth/infrastructure/SessionInvalidator.java (1)
15-19:⚠️ Potential issue | 🔴 Critical
logout에서 세션/쿠키 부재 시 NPE로 500이 발생합니다.Line 15의
request.getSession(false).invalidate()와 Line 16의Arrays.stream(request.getCookies())는 각각 null 반환 시 NPE가 발생하여, Line 19의BadRequestException으로 처리되지 않고 500 에러가 발생합니다.🔧 제안 수정안
public static void logout(HttpServletRequest request, HttpServletResponse response) { - request.getSession(false).invalidate(); - Cookie jSessionIdCookie = Arrays.stream(request.getCookies()) + if (request.getSession(false) == null) { + throw new BadRequestException("활성 세션이 존재하지 않습니다."); + } + request.getSession(false).invalidate(); + + Cookie[] cookies = request.getCookies(); + if (cookies == null) { + throw new BadRequestException("JSESSIONID 쿠키가 요청에 존재하지 않습니다."); + } + Cookie jSessionIdCookie = Arrays.stream(cookies) .filter(cookie -> "JSESSIONID".equals(cookie.getName())) .findFirst() .orElseThrow(() -> new BadRequestException("JSESSIONID 쿠키가 요청에 존재하지 않습니다."));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/auth/infrastructure/SessionInvalidator.java` around lines 15 - 19, The logout flow in SessionInvalidator currently calls request.getSession(false).invalidate() and Arrays.stream(request.getCookies()) without null checks, causing NPEs instead of the intended BadRequestException; update the logout/session invalidation logic in SessionInvalidator to first retrieve HttpSession session = request.getSession(false) and only call session.invalidate() if session != null, and similarly guard request.getCookies() by treating a null return as an empty array (or explicitly checking for null and then throwing the existing BadRequestException for missing JSESSIONID), then proceed to stream cookies and find JSESSIONID so missing-session or missing-cookie cases yield the BadRequestException rather than a 500 NPE.
♻️ Duplicate comments (1)
src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java (1)
65-90:⚠️ Potential issue | 🟠 Major생성자 검증이
nullable = false필드를 모두 보장하지 못합니다.Line 67~74에서
orderName,metadata,serviceType는 대입되지만 생성자 검증에서 누락되어, DB 제약에 도달하기 전 엔티티 불변식이 깨질 수 있습니다.제안 수정안
public TosspaymentsPayment(String paymentKey, Long memberId, String orderId, String orderName, Long totalAmount, String metadata, ServiceType serviceType) { - validateConstructorParams(paymentKey, memberId, orderId, totalAmount); + validateConstructorParams(paymentKey, memberId, orderId, orderName, totalAmount, metadata, serviceType); this.paymentKey = paymentKey; this.memberId = memberId; this.orderId = orderId; this.orderName = orderName; this.totalAmount = totalAmount; this.metadata = metadata; this.serviceType = serviceType; this.state = PaymentState.NEED_APPROVE; } - private void validateConstructorParams(String paymentKey, Long memberId, String orderId, Long totalAmount) { + private void validateConstructorParams(String paymentKey, Long memberId, String orderId, String orderName, + Long totalAmount, String metadata, ServiceType serviceType) { if (paymentKey == null || paymentKey.isBlank()) { throw new BadRequestException("paymentKey는 필수입니다."); } if (memberId == null) { throw new BadRequestException("memberId는 필수입니다."); } if (orderId == null || orderId.isBlank()) { throw new BadRequestException("orderId는 필수입니다."); } + if (orderName == null || orderName.isBlank()) { + throw new BadRequestException("orderName은 필수입니다."); + } if (totalAmount == null || totalAmount < 0) { throw new BadRequestException("totalAmount는 0 이상이어야 합니다."); } + if (metadata == null || metadata.isBlank()) { + throw new BadRequestException("metadata는 필수입니다."); + } + if (serviceType == null) { + throw new BadRequestException("serviceType은 필수입니다."); + } }As per coding guidelines: "Apply
@Validannotation in DTOs for validation and implement entity-level validation in constructors".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java` around lines 65 - 90, The constructor TosspaymentsPayment and its helper validateConstructorParams currently validate paymentKey, memberId, orderId, and totalAmount but omit orderName, metadata, and serviceType, which can violate DB non-null constraints; update validateConstructorParams to check orderName (non-null/non-blank), metadata (non-null or as-per-domain allow blank?—if DB nullable=false treat as non-null/non-blank) and serviceType (non-null) and throw BadRequestException with clear messages when they are invalid so the TosspaymentsPayment(...) constructor enforces entity invariants before persisting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/samhap/kokomen/interview/service/RootQuestionService.java`:
- Around line 49-53: The method findFirstRootQuestion throws a NotFoundException
when no value is present, so rename it to readFirstRootQuestion to follow the
read-/find- naming convention; update its declaration (previously
findFirstRootQuestion) and all call sites (including the caller that currently
invokes it) to use readFirstRootQuestion, leaving the body unchanged (it still
calls
rootQuestionRepository.findRootQuestionByCategoryAndStateAndQuestionOrder(...)
and orElseThrow(...)). Ensure imports and any references to the old method name
are updated to avoid compilation errors.
In `@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java`:
- Around line 93-95: The updateState(PaymentState state) setter allows null and
can violate the non-null DB constraint; modify the updateState method to
validate the incoming state parameter and throw a BadRequestException (or other
appropriate custom exception) when state is null, preserving object/DB integrity
and using the project's custom exceptions rather than letting a DB error surface
from the state field.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.java`:
- Around line 10-15: The constructor TosspaymentsPaymentCancelRequest currently
uses Objects.requireNonNull(cancelReason, ...) which throws NullPointerException
and breaks exception consistency; replace that call with an explicit null check
that throws BadRequestException when cancelReason is null, keep the existing
blank check that throws BadRequestException, and remove the now-unused
java.util.Objects import.
In `@src/main/java/com/samhap/kokomen/token/domain/Token.java`:
- Around line 49-52: In Token.addTokens(int count) the validation message says
the count must be greater than 0 but the condition only rejects negative values
(count < 0), so make them consistent: either change the guard to reject
non‑positive values (count <= 0) if zero should be disallowed, or update the
BadRequestException message to state "0 이상이어야 합니다" (or equivalent) if zero
should be allowed; adjust the condition and the BadRequestException text in the
addTokens method accordingly.
In `@src/main/java/com/samhap/kokomen/token/service/TokenService.java`:
- Around line 31-35: Validate that the incoming count is a positive integer at
the start of token-modifying service methods (e.g., addPaidTokens and the
corresponding method around lines 69-73 that calls
tokenRepository.incrementTokenCountModifying) and immediately throw a
BadRequestException when count <= 0; then proceed with the existing increment
and existing InternalServerErrorException handling if updatedRows == 0. Include
memberId and the invalid count in the BadRequestException message to aid
debugging.
---
Outside diff comments:
In
`@src/main/java/com/samhap/kokomen/auth/infrastructure/SessionInvalidator.java`:
- Around line 15-19: The logout flow in SessionInvalidator currently calls
request.getSession(false).invalidate() and Arrays.stream(request.getCookies())
without null checks, causing NPEs instead of the intended BadRequestException;
update the logout/session invalidation logic in SessionInvalidator to first
retrieve HttpSession session = request.getSession(false) and only call
session.invalidate() if session != null, and similarly guard
request.getCookies() by treating a null return as an empty array (or explicitly
checking for null and then throwing the existing BadRequestException for missing
JSESSIONID), then proceed to stream cookies and find JSESSIONID so
missing-session or missing-cookie cases yield the BadRequestException rather
than a 500 NPE.
In `@src/main/java/com/samhap/kokomen/global/service/S3Service.java`:
- Around line 69-73: In extractKeyFromCdnUrl, guard against null before calling
startsWith so a BadRequestException is thrown for null or invalid cdnUrl values;
update the method (reference: extractKeyFromCdnUrl and
AwsConstant.CLOUD_FRONT_DOMAIN_URL) to first check cdnUrl for null/empty and
throw new BadRequestException("Invalid CDN URL: " + cdnUrl) when it fails,
otherwise perform the substring operation so no NPE occurs.
In
`@src/main/java/com/samhap/kokomen/recruit/schedular/service/RecruitmentApiClient.java`:
- Around line 110-123: The validateResponse method currently throws
BadRequestException for null/invalid external API responses which misuses HTTP
400; change validateResponse to throw a more appropriate server-side exception
(e.g., create and throw ExternalApiException or IllegalStateException) so
external API failures are represented as 5xx; update the code to define
ExternalApiException (extending KokomenException with a 503/Service Unavailable
status) and replace BadRequestException throws in validateResponse, and ensure
fetchPageSafely continues to catch and handle that exception as before.
---
Duplicate comments:
In `@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java`:
- Around line 65-90: The constructor TosspaymentsPayment and its helper
validateConstructorParams currently validate paymentKey, memberId, orderId, and
totalAmount but omit orderName, metadata, and serviceType, which can violate DB
non-null constraints; update validateConstructorParams to check orderName
(non-null/non-blank), metadata (non-null or as-per-domain allow blank?—if DB
nullable=false treat as non-null/non-blank) and serviceType (non-null) and throw
BadRequestException with clear messages when they are invalid so the
TosspaymentsPayment(...) constructor enforces entity invariants before
persisting.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/main/java/com/samhap/kokomen/answer/service/AnswerService.javasrc/main/java/com/samhap/kokomen/auth/infrastructure/SessionInvalidator.javasrc/main/java/com/samhap/kokomen/global/infrastructure/MemberAuthArgumentResolver.javasrc/main/java/com/samhap/kokomen/global/service/S3Service.javasrc/main/java/com/samhap/kokomen/interview/domain/QuestionAndAnswers.javasrc/main/java/com/samhap/kokomen/interview/service/RootQuestionService.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.javasrc/main/java/com/samhap/kokomen/recruit/schedular/service/RecruitmentApiClient.javasrc/main/java/com/samhap/kokomen/token/domain/Token.javasrc/main/java/com/samhap/kokomen/token/domain/TokenPurchase.javasrc/main/java/com/samhap/kokomen/token/service/TokenService.javasrc/test/java/com/samhap/kokomen/interview/domain/QuestionAndAnswersTest.java
Comment on lines
49
to
53
| private RootQuestion findFirstRootQuestion(Category category) { | ||
| return rootQuestionRepository.findRootQuestionByCategoryAndStateAndQuestionOrder(category, | ||
| RootQuestionState.ACTIVE, 1) | ||
| .orElseThrow(() -> new IllegalStateException("해당 카테고리의 질문을 찾을 수 없습니다.")); | ||
| .orElseThrow(() -> new NotFoundException("해당 카테고리의 질문을 찾을 수 없습니다.")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
예외 발생 메서드인데 find- prefix를 사용하고 있습니다
Line 49의 findFirstRootQuestion는 Line 52에서 값 부재 시 예외를 던지므로 find- 규칙과 맞지 않습니다. readFirstRootQuestion로 명명하고 호출부(Line 46)도 함께 변경해 주세요.
제안 diff
- private RootQuestion findFirstRootQuestion(Category category) {
+ private RootQuestion readFirstRootQuestion(Category category) {
return rootQuestionRepository.findRootQuestionByCategoryAndStateAndQuestionOrder(category,
RootQuestionState.ACTIVE, 1)
.orElseThrow(() -> new NotFoundException("해당 카테고리의 질문을 찾을 수 없습니다."));
}
@@
- .orElseGet(() -> findFirstRootQuestion(category));
+ .orElseGet(() -> readFirstRootQuestion(category));As per coding guidelines, Use 'read-' prefix for methods where values must exist and throw exceptions if not found; Use 'find-' prefix for methods where values may not exist, returning Optional or empty list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/interview/service/RootQuestionService.java`
around lines 49 - 53, The method findFirstRootQuestion throws a
NotFoundException when no value is present, so rename it to
readFirstRootQuestion to follow the read-/find- naming convention; update its
declaration (previously findFirstRootQuestion) and all call sites (including the
caller that currently invokes it) to use readFirstRootQuestion, leaving the body
unchanged (it still calls
rootQuestionRepository.findRootQuestionByCategoryAndStateAndQuestionOrder(...)
and orElseThrow(...)). Ensure imports and any references to the old method name
are updated to avoid compilation errors.
Comment on lines
+93
to
+95
| public void updateState(PaymentState state) { | ||
| this.state = state; | ||
| } |
There was a problem hiding this comment.
updateState에 null 방어가 없어 상태 무결성이 깨질 수 있습니다.
Line 94에서 state를 그대로 대입해 null이 들어오면 nullable = false 필드가 깨지고, 오류가 DB 단계로 지연됩니다.
제안 수정안
public void updateState(PaymentState state) {
+ if (state == null) {
+ throw new BadRequestException("state는 필수입니다.");
+ }
this.state = state;
}As per coding guidelines: "Use custom exceptions: BadRequestException, UnauthorizedException, ForbiddenException, etc. for exception handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java`
around lines 93 - 95, The updateState(PaymentState state) setter allows null and
can violate the non-null DB constraint; modify the updateState method to
validate the incoming state parameter and throw a BadRequestException (or other
appropriate custom exception) when state is null, preserving object/DB integrity
and using the project's custom exceptions rather than letting a DB error surface
from the state field.
Comment on lines
+10
to
+15
| public TosspaymentsPaymentCancelRequest { | ||
| Objects.requireNonNull(cancelReason, "cancelReason은 필수입니다."); | ||
| if (cancelReason.isBlank()) { | ||
| throw new BadRequestException("cancelReason은 비어있을 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Objects.requireNonNull은 NullPointerException을 던지므로 예외 일관성이 깨집니다.
Line 11의 Objects.requireNonNull은 null일 때 NullPointerException을 던지지만, Line 13의 blank 검증은 BadRequestException을 던집니다. 코딩 가이드라인에 따라 모든 검증 예외를 BadRequestException으로 통일해야 합니다.
🛡️ 예외 타입 통일을 위한 수정 제안
public TosspaymentsPaymentCancelRequest {
- Objects.requireNonNull(cancelReason, "cancelReason은 필수입니다.");
+ if (cancelReason == null) {
+ throw new BadRequestException("cancelReason은 필수입니다.");
+ }
if (cancelReason.isBlank()) {
throw new BadRequestException("cancelReason은 비어있을 수 없습니다.");
}
}수정 후 java.util.Objects import는 제거 가능합니다.
As per coding guidelines, "Use custom exceptions: BadRequestException, UnauthorizedException, ForbiddenException, etc. for exception handling".
📝 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 TosspaymentsPaymentCancelRequest { | |
| Objects.requireNonNull(cancelReason, "cancelReason은 필수입니다."); | |
| if (cancelReason.isBlank()) { | |
| throw new BadRequestException("cancelReason은 비어있을 수 없습니다."); | |
| } | |
| } | |
| public TosspaymentsPaymentCancelRequest { | |
| if (cancelReason == null) { | |
| throw new BadRequestException("cancelReason은 필수입니다."); | |
| } | |
| if (cancelReason.isBlank()) { | |
| throw new BadRequestException("cancelReason은 비어있을 수 없습니다."); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/samhap/kokomen/payment/external/dto/TosspaymentsPaymentCancelRequest.java`
around lines 10 - 15, The constructor TosspaymentsPaymentCancelRequest currently
uses Objects.requireNonNull(cancelReason, ...) which throws NullPointerException
and breaks exception consistency; replace that call with an explicit null check
that throws BadRequestException when cancelReason is null, keep the existing
blank check that throws BadRequestException, and remove the now-unused
java.util.Objects import.
Comment on lines
49
to
52
| public void addTokens(int count) { | ||
| if (count < 0) { | ||
| throw new IllegalStateException("추가할 토큰 수는 0보다 커야 합니다."); | ||
| throw new BadRequestException("추가할 토큰 수는 0보다 커야 합니다."); | ||
| } |
There was a problem hiding this comment.
검증 조건과 예외 메시지가 불일치합니다.
Line 50은 count < 0만 막아서 0은 허용되는데, Line 51 메시지는 0보다 커야라고 안내합니다. 조건 또는 메시지 중 하나를 맞춰주세요.
수정 예시
- throw new BadRequestException("추가할 토큰 수는 0보다 커야 합니다.");
+ throw new BadRequestException("추가할 토큰 수는 0 이상이어야 합니다.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/token/domain/Token.java` around lines 49 -
52, In Token.addTokens(int count) the validation message says the count must be
greater than 0 but the condition only rejects negative values (count < 0), so
make them consistent: either change the guard to reject non‑positive values
(count <= 0) if zero should be disallowed, or update the BadRequestException
message to state "0 이상이어야 합니다" (or equivalent) if zero should be allowed; adjust
the condition and the BadRequestException text in the addTokens method
accordingly.
Comment on lines
31
to
35
| public void addPaidTokens(Long memberId, int count) { | ||
| int updatedRows = tokenRepository.incrementTokenCountModifying(memberId, TokenType.PAID, count); | ||
| if (updatedRows == 0) { | ||
| throw new IllegalStateException("유료 토큰 구매에 실패했습니다. memberId: " + memberId); | ||
| throw new InternalServerErrorException("유료 토큰 구매에 실패했습니다. memberId: " + memberId); | ||
| } |
There was a problem hiding this comment.
count 양수 검증이 없어 토큰 무결성이 깨질 수 있습니다.
Line 31-35와 Line 69-73에서 count <= 0 검증이 없어, 잘못된 입력으로 토큰이 반대로 증감될 수 있습니다. 서비스 레벨에서 즉시 차단해야 합니다.
수정 예시
`@Transactional`
public void addPaidTokens(Long memberId, int count) {
+ if (count <= 0) {
+ throw new BadRequestException("추가할 유료 토큰 수는 0보다 커야 합니다.");
+ }
int updatedRows = tokenRepository.incrementTokenCountModifying(memberId, TokenType.PAID, count);
if (updatedRows == 0) {
throw new InternalServerErrorException("유료 토큰 구매에 실패했습니다. memberId: " + memberId);
}
}
`@Transactional`
public void refundPaidTokenCount(Long memberId, int count) {
+ if (count <= 0) {
+ throw new BadRequestException("환불할 유료 토큰 수는 0보다 커야 합니다.");
+ }
int updatedRows = tokenRepository.decrementTokenCountModifying(memberId, TokenType.PAID, count);
if (updatedRows == 0) {
throw new InternalServerErrorException("유료 토큰 환불에 실패했습니다. memberId: " + memberId);
}
}As per coding guidelines "Use custom exceptions: BadRequestException, UnauthorizedException, ForbiddenException, etc. for exception handling".
Also applies to: 69-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/samhap/kokomen/token/service/TokenService.java` around
lines 31 - 35, Validate that the incoming count is a positive integer at the
start of token-modifying service methods (e.g., addPaidTokens and the
corresponding method around lines 69-73 that calls
tokenRepository.incrementTokenCountModifying) and immediately throw a
BadRequestException when count <= 0; then proceed with the existing increment
and existing InternalServerErrorException handling if updatedRows == 0. Include
memberId and the invalid count in the BadRequestException message to aid
debugging.
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 #331
작업 내용
스크린샷
참고 사항