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. 📝 WalkthroughWalkthrough결제 도메인·서비스 예외 처리와 메시지를 중앙화하고, 결제 확인/취소 흐름에서 에러 분기와 상태 전이를 정교화했으며, CI 워크플로우의 조건부 빌드 제거, 테스트 픽스처 및 통합 테스트 추가, Docker Compose 구성 재구성 등을 적용했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentFacade as PaymentFacadeService
participant TossClient as TosspaymentsClient
participant Repo as Repository
participant DB as Database
Client->>PaymentFacade: confirmPayment(request)
PaymentFacade->>Repo: findPaymentById/updateState(NEED_CANCEL or ... )
PaymentFacade->>TossClient: confirmPayment(request)
alt Http 200 OK
TossClient-->>PaymentFacade: successResponse
PaymentFacade->>Repo: persistResult/updateState(COMPLETED)
PaymentFacade-->>Client: successResponse
else Http 4xx (client)
TossClient-->>PaymentFacade: HttpClientErrorException
PaymentFacade->>Repo: updateState(SERVER_BAD_REQUEST or CLIENT_BAD_REQUEST)
PaymentFacade-->>Client: throw BadRequestException / InternalServerErrorException
else Http 5xx
TossClient-->>PaymentFacade: HttpServerErrorException
PaymentFacade->>Repo: updateState(NEED_CANCEL)
PaymentFacade-->>Client: rethrow (InternalServerErrorException)
else Network Error
TossClient-->>PaymentFacade: ResourceAccessException (SocketTimeout)
PaymentFacade->>Repo: updateState(CONNECTION_TIMEOUT or NEED_CANCEL)
PaymentFacade-->>Client: rethrow (InternalServerErrorException)
end
Estimated code review effort🎯 4 (복잡) | ⏱️ ~45분 Possibly related issues
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는 결제 처리 로직을 대대적으로 리팩토링하여 오류 처리의 일관성과 예외 관리의 명확성을 높이는 데 중점을 두었습니다. 오류 메시지를 중앙 집중화하고, 특정 예외 유형을 도입하며, 다양한 실패 시나리오를 보다 효과적으로 처리하도록 결제 승인 및 취소 흐름을 개선했습니다. 또한, 새로운 GitHub 템플릿 도입과 테스트 환경 설정 업데이트를 통해 개발 워크플로우를 간소화했습니다. 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
Ignored Files
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 Results1 tests 1 ✅ 0s ⏱️ Results for commit 2f29348. ♻️ 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 PR performs a large-scale refactoring of payment-related code. However, it introduces significant security risks related to broken access control and insecure configuration in the payment module. The internal payment service lacks ownership checks for payment cancellation and confirmation and is exposed to the host network without authentication, potentially allowing attackers to manipulate or cancel payments. It is highly recommended to implement proper authorization checks and restrict access to internal services to mitigate these risks. On the code quality front, error handling logic is robust, and consistency and maintainability have improved with standardized exceptions and error messages, along with extensive high-quality test coverage. A few minor improvements were also suggested during the review.
| } | ||
| } | ||
|
|
||
| public void cancelPayment(CancelRequest request) { |
There was a problem hiding this comment.
The cancelPayment method accepts a paymentKey and proceeds to cancel the payment without verifying if the payment belongs to the authenticated user. This is an Insecure Direct Object Reference (IDOR) vulnerability. An attacker who knows or guesses a paymentKey can cancel any user's payment, leading to a Denial of Service on the payment system.
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | ||
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | ||
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | ||
| message = String.format( | ||
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | ||
| fieldName, | ||
| invalidValue | ||
| ); | ||
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorResponse(ApiErrorMessage.JSON_PARSE_ERROR.getMessage())); | ||
| } |
There was a problem hiding this comment.
InvalidFormatException 발생 시 API 응답으로 일반적인 오류 메시지를 반환하도록 변경되었는데, 이는 API를 사용하는 개발자 입장에서 디버깅을 어렵게 만들 수 있습니다.
이전 구현처럼 어떤 필드에서 어떤 값 때문에 오류가 발생했는지에 대한 구체적인 정보를 포함하여 응답하는 것을 고려해 보세요. 민감한 정보를 노출하지 않는 선에서, 개발자에게 유용한 정보를 제공하는 것이 좋습니다.
예를 들어, 다음과 같이 상세 메시지를 생성하여 반환할 수 있습니다.
if (e.getCause() instanceof InvalidFormatException invalidFormatException) {
String fieldName = invalidFormatException.getPath().get(0).getFieldName();
String invalidValue = String.valueOf(invalidFormatException.getValue());
log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue);
String detailedMessage = String.format("JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: %s)", fieldName, invalidValue);
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
.body(new ErrorResponse(detailedMessage));
}
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | |
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | |
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | |
| message = String.format( | |
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | |
| fieldName, | |
| invalidValue | |
| ); | |
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body(new ErrorResponse(ApiErrorMessage.JSON_PARSE_ERROR.getMessage())); | |
| } | |
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | |
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | |
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | |
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | |
| String detailedMessage = String.format("JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: %s)", fieldName, invalidValue); | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body(new ErrorResponse(detailedMessage)); | |
| } |
| package com.samhap.kokomen.global.exception; | ||
|
|
||
| import lombok.Getter; | ||
|
|
||
| @Getter | ||
| public enum ExternalErrorMessage { | ||
|
|
||
| INVALID_REQUEST("잘못된 요청입니다."), | ||
| MISSING_REQUEST_PARAMETER("필수 요청 파라미터가 누락되었습니다."), | ||
| INVALID_REQUEST_FORMAT("잘못된 요청 형식입니다. JSON 형식을 확인해주세요."), | ||
| JSON_PARSE_ERROR("JSON 파싱 오류: 유효하지 않은 값이 전달되었습니다."), | ||
| INTERNAL_SERVER_ERROR("서버에 문제가 발생하였습니다."); | ||
|
|
||
| private final String message; | ||
|
|
||
| ExternalErrorMessage(String message) { | ||
| this.message = message; | ||
| } | ||
| } |
There was a problem hiding this comment.
ExternalErrorMessage 열거형의 내용이 api 모듈의 ApiErrorMessage와 거의 중복됩니다. 이러한 공통 오류 메시지는 common 모듈로 옮겨서 중복을 제거하는 것이 좋겠습니다.
예를 들어, common 모듈에 CommonErrorMessage를 만들고, 각 모듈에서 이를 사용하거나 확장하는 방식을 고려해볼 수 있습니다. 이렇게 하면 향후 오류 메시지를 일관되게 관리하기 용이해집니다.
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | ||
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | ||
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | ||
| message = String.format( | ||
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | ||
| fieldName, | ||
| invalidValue | ||
| ); | ||
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorResponse(ExternalErrorMessage.JSON_PARSE_ERROR.getMessage())); | ||
| } |
There was a problem hiding this comment.
api 모듈의 GlobalExceptionHandler와 동일한 문제입니다. InvalidFormatException 발생 시 API 응답으로 일반적인 오류 메시지를 반환하고 있어 디버깅이 어려울 수 있습니다.
어떤 필드에서 오류가 발생했는지 구체적인 정보를 응답에 포함하는 것을 고려해 보세요.
if (e.getCause() instanceof InvalidFormatException invalidFormatException) {
String fieldName = invalidFormatException.getPath().get(0).getFieldName();
String invalidValue = String.valueOf(invalidFormatException.getValue());
log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue);
String detailedMessage = String.format("JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: %s)", fieldName, invalidValue);
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
.body(new ErrorResponse(detailedMessage));
}
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | |
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | |
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | |
| message = String.format( | |
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | |
| fieldName, | |
| invalidValue | |
| ); | |
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body(new ErrorResponse(ExternalErrorMessage.JSON_PARSE_ERROR.getMessage())); | |
| } | |
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | |
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | |
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | |
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | |
| String detailedMessage = String.format("JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: %s)", fieldName, invalidValue); | |
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | |
| .body(new ErrorResponse(detailedMessage)); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@api/src/main/java/com/samhap/kokomen/global/exception/ApiErrorMessage.java`:
- Line 9: The enum constant LOGIN_REQUIRED in ApiErrorMessage is missing a
trailing period; update the enum entry LOGIN_REQUIRED("로그인이 필요합니다") to include a
final period in its message so it matches the punctuation style of the other
enum values (e.g., LOGIN_REQUIRED("로그인이 필요합니다.")). Ensure no other enum usages
or tests rely on the exact string without the period.
In
`@api/src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java`:
- Around line 56-62: In GlobalExceptionHandler where you inspect
InvalidFormatException (the block using e.getCause() instanceof
InvalidFormatException invalidFormatException), guard against an empty path
before calling invalidFormatException.getPath().get(0): check if
invalidFormatException.getPath() is null or empty and only extract fieldName and
value when present; otherwise set a safe fallback (e.g., "unknown" or omit
fieldName) and still log the invalidValue. Update the log.warn call to use the
fallback fieldName and keep returning the same BAD_REQUEST ErrorResponse so
behavior is preserved.
In `@domain/build.gradle`:
- Line 2: Change the dependency declaration in the domain module from an
implementation to an api dependency so types from common are exposed to domain
consumers: replace the current implementation project(':common') in domain's
build.gradle with an api configuration; this ensures the domain method
TosspaymentsPayment.validateTosspaymentsResult() can legitimately throw and
expose common.InternalServerErrorException to modules depending on domain.
In
`@external/src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java`:
- Around line 56-62: In GlobalExceptionHandler where you handle
HttpMessageNotReadableException and cast e.getCause() to InvalidFormatException
(invalidFormatException), guard against an empty path before calling
invalidFormatException.getPath().get(0) by checking
invalidFormatException.getPath() != null &&
!invalidFormatException.getPath().isEmpty(); if empty, use a safe fallback
(e.g., "unknown" or skip fieldName/invalidValue extraction) and still log and
return the same ErrorResponse; update the logic around fieldName and
invalidValue extraction in that block to avoid IndexOutOfBoundsException.
In
`@internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java`:
- Around line 69-82: handleConfirmClientError (and the similar logic in
cancelPayment) can NPE when e.getResponseBodyAs(Failure.class) returns null; add
a null check after calling getResponseBodyAs and treat a null Failure as an
unknown/empty response by (1) logging that the response body was missing or
unparsable along with the original exception, (2) using a safe default for
code/message (e.g. "UNKNOWN" and e.getMessage() or a default message) when
evaluating TosspaymentsInternalServerErrorCode and when creating the returned
exception, and (3) still updating tosspaymentsPaymentService.updateState(...)
appropriately (choose SERVER_BAD_REQUEST only if code matches, otherwise
CLIENT_BAD_REQUEST or a generic error state). Apply the same null-guard pattern
to the cancelPayment handling at the referenced lines.
- Around line 96-108: handleConfirmNetworkError currently updates state only for
SocketTimeoutException and leaves other network failures (e.g.,
ConnectException, UnknownHostException, generic IOException) unhandled, so
confirmPayment rethrows without state change; modify handleConfirmNetworkError
to detect other network-related root causes (check e.getRootCause() instanceof
ConnectException, UnknownHostException or IOException) and call
tosspaymentsPaymentService.updateState(tosspaymentsPayment.getId(),
PaymentState.CONNECTION_TIMEOUT) (or PaymentState.NEED_CANCEL where appropriate)
before allowing the exception to propagate; ensure the method
(handleConfirmNetworkError) performs the state update for all network IO
failures so confirmPayment’s exception path doesn’t leave the payment in the
initial state.
🧹 Nitpick comments (9)
common/run-test-redis.sh (1)
1-30:run-test-mysql.sh와run-test-redis.sh의 헬스체크 로직 중복 고려.두 스크립트의 헬스체크 대기 루프가 거의 동일한 구조입니다. 향후 서비스가 더 추가될 경우, 공통 헬스체크 함수를 별도 스크립트로 추출하면 유지보수가 편해질 수 있습니다. 현재 수준에서는 허용 가능한 중복입니다.
.github/workflows/cd-internal-prod.yml (1)
82-93:REDIS_PRIMARY_HOST_PROD환경 변수가 deploy 단계의env블록에 누락되어 있습니다.
docker-compose-prod.yml에서kokomen-payment-api와kokomen-payment-internal모두REDIS_PRIMARY_HOST_PROD를 참조하고 있지만, 이 워크플로우의env블록(Line 84-89)에는 해당 변수가 포함되어 있지 않습니다. 이 변수가 self-hosted runner의 시스템 환경에 별도로 설정되어 있다면 문제없겠지만, 그렇지 않다면 Redis 연결이 실패할 수 있습니다.이 PR에서 추가된 변경은 아니지만, 관련 블록을 수정하는 김에 함께 확인해 보시면 좋겠습니다.
common/src/main/java/com/samhap/kokomen/global/exception/NotFoundException.java (1)
3-7:InternalServerErrorException와의 일관성 부족:Throwable cause생성자 누락 및 들여쓰기 차이.
InternalServerErrorException은(String message, Throwable cause)생성자를 제공하지만,NotFoundException에는 해당 생성자가 없습니다. 원인 체이닝이 필요한 경우를 대비해 추가를 고려해주세요. 또한 들여쓰기가 2칸으로,InternalServerErrorException의 4칸과 다릅니다.external/src/main/java/com/samhap/kokomen/global/exception/ExternalErrorMessage.java (1)
8-12:ApiErrorMessage와 메시지 중복.
INVALID_REQUEST,MISSING_REQUEST_PARAMETER,INVALID_REQUEST_FORMAT,JSON_PARSE_ERROR,INTERNAL_SERVER_ERROR5개 항목이ApiErrorMessage와 동일한 메시지 문자열을 가지고 있습니다. 추후 메시지 변경 시 동기화 누락 위험이 있으므로, 공통 메시지를common모듈로 추출하는 것을 고려해주세요.api/src/main/java/com/samhap/kokomen/global/infrastructure/MemberAuthArgumentResolver.java (2)
37-38:getNativeRequest()가null을 반환할 경우 NPE 발생 가능.
webRequest.getNativeRequest(HttpServletRequest.class)는 이론적으로null을 반환할 수 있으며, Line 38에서request.getSession(false)호출 시 NPE가 발생합니다. Spring MVC 환경에서는 일반적으로null이 아니지만, 방어적 null 체크 또는Objects.requireNonNull사용을 고려해주세요.
56-62:memberId가null이고 인증이 필수가 아닌 경우에도 error 레벨로 로깅.Line 57-58에서
memberId == null이면authenticationRequired여부와 관계없이log.error를 호출합니다. 선택적 인증(optional auth) 시 세션은 있지만MEMBER_ID가 없는 것이 정상 시나리오일 수 있다면,authenticationRequired에 따라 로그 레벨을 분리하는 것이 적절합니다.♻️ 수정 제안
private void validateAuthentication(Long memberId, boolean authenticationRequired) { - if (memberId == null) { - log.error(ApiErrorMessage.MEMBER_ID_NOT_IN_SESSION.getMessage()); - } if (memberId == null && authenticationRequired) { + log.error(ApiErrorMessage.MEMBER_ID_NOT_IN_SESSION.getMessage()); throw new UnauthorizedException(ApiErrorMessage.MEMBER_ID_NOT_IN_SESSION.getMessage()); + } else if (memberId == null) { + log.warn(ApiErrorMessage.MEMBER_ID_NOT_IN_SESSION.getMessage()); } }domain/src/test/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResultTest.java (1)
13-21:TosspaymentsPaymentResult생성 시 23개의 파라미터 사용 — Fixture Builder 도입 고려
TosspaymentsPaymentRepository테스트에서는TosspaymentsPaymentFixtureBuilder를 사용하고 있지만, 여기서는 23개의 생성자 파라미터를 직접 전달하고 있어 가독성이 떨어지고 유지보수가 어렵습니다.TosspaymentsPaymentResultFixtureBuilder도입을 고려해 보세요.domain/src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java (1)
17-19: 도메인 엔티티에@Slf4j사용 — 설계 트레이드오프 인지도메인 엔티티에 로깅 프레임워크 의존성을 추가하는 것은 순수 도메인 모델 원칙에서 벗어나지만, 결제 검증 실패라는 심각한 상황에서의 진단 로깅이라는 점에서 실용적인 선택입니다. 향후 도메인 순수성이 중요해지면, 검증 로직을 도메인 서비스로 분리하는 것도 방법입니다.
internal/src/main/java/com/samhap/kokomen/global/exception/PaymentServiceErrorMessage.java (1)
6-16:api모듈의ApiErrorMessage와 중복되는 에러 메시지 상수 존재
INVALID_REQUEST,INTERNAL_SERVER_ERROR상수가api모듈의ApiErrorMessage에 동일한 메시지로 존재합니다. 두 모듈에서 이 메시지들을 공통으로 사용한다면,common모듈로 추출하여 일관성을 유지하는 것을 권장합니다. 다만INVALID_REQUEST_FORMAT은 각 모듈에서 제공하는 메시지가 다르므로(API 모듈은 JSON 형식 안내 포함), 필요에 따라 구분해서 사용할 수 있습니다.
| public enum ApiErrorMessage { | ||
|
|
||
| AUTHENTICATION_ANNOTATION_REQUIRED("MemberAuth 파라미터는 @Authentication 어노테이션이 있어야 합니다."), | ||
| LOGIN_REQUIRED("로그인이 필요합니다"), |
There was a problem hiding this comment.
LOGIN_REQUIRED 메시지에 마침표(.) 누락.
다른 메시지들은 모두 마침표로 끝나지만, LOGIN_REQUIRED("로그인이 필요합니다")만 마침표가 없습니다.
✏️ 수정 제안
- LOGIN_REQUIRED("로그인이 필요합니다"),
+ LOGIN_REQUIRED("로그인이 필요합니다."),
📝 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.
| LOGIN_REQUIRED("로그인이 필요합니다"), | |
| LOGIN_REQUIRED("로그인이 필요합니다."), |
🤖 Prompt for AI Agents
In `@api/src/main/java/com/samhap/kokomen/global/exception/ApiErrorMessage.java`
at line 9, The enum constant LOGIN_REQUIRED in ApiErrorMessage is missing a
trailing period; update the enum entry LOGIN_REQUIRED("로그인이 필요합니다") to include a
final period in its message so it matches the punctuation style of the other
enum values (e.g., LOGIN_REQUIRED("로그인이 필요합니다.")). Ensure no other enum usages
or tests rely on the exact string without the period.
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | ||
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | ||
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | ||
| message = String.format( | ||
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | ||
| fieldName, | ||
| invalidValue | ||
| ); | ||
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorResponse(ApiErrorMessage.JSON_PARSE_ERROR.getMessage())); | ||
| } |
There was a problem hiding this comment.
invalidFormatException.getPath().get(0)에서 IndexOutOfBoundsException 발생 가능
InvalidFormatException의 getPath()가 빈 리스트를 반환하는 경우 get(0) 호출 시 IndexOutOfBoundsException이 발생할 수 있습니다. path가 비어있는 경우에 대한 방어 로직이 필요합니다.
🛡️ 방어 코드 제안
if (e.getCause() instanceof InvalidFormatException invalidFormatException) {
- String fieldName = invalidFormatException.getPath().get(0).getFieldName();
- String invalidValue = String.valueOf(invalidFormatException.getValue());
- log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue);
+ var path = invalidFormatException.getPath();
+ if (!path.isEmpty()) {
+ String fieldName = path.get(0).getFieldName();
+ String invalidValue = String.valueOf(invalidFormatException.getValue());
+ log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue);
+ } else {
+ log.warn("HttpMessageNotReadableException :: invalidValue: {}", invalidFormatException.getValue());
+ }
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
.body(new ErrorResponse(ApiErrorMessage.JSON_PARSE_ERROR.getMessage()));
}🤖 Prompt for AI Agents
In
`@api/src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java`
around lines 56 - 62, In GlobalExceptionHandler where you inspect
InvalidFormatException (the block using e.getCause() instanceof
InvalidFormatException invalidFormatException), guard against an empty path
before calling invalidFormatException.getPath().get(0): check if
invalidFormatException.getPath() is null or empty and only extract fieldName and
value when present; otherwise set a safe fallback (e.g., "unknown" or omit
fieldName) and still log the invalidValue. Update the log.warn call to use the
fallback fieldName and keep returning the same BAD_REQUEST ErrorResponse so
behavior is preserved.
| @@ -1,4 +1,5 @@ | |||
| dependencies { | |||
| implementation project(':common') | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking domain module imports from common ==="
if [ -d "domain/src/main" ]; then
find domain/src/main -type f \( -name "*.java" -o -name "*.kt" \) -exec grep -l "import.*common\." {} \; 2>/dev/null || echo "No imports found"
else
echo "domain/src/main directory not found"
fi
echo ""
echo "=== All modules that depend on common ==="
rg -n "project.*:common" --glob 'build.gradle*'
echo ""
echo "=== Domain module build.gradle content ==="
if [ -f "domain/build.gradle" ]; then
cat -n domain/build.gradle
fi
echo ""
echo "=== Checking domain module structure ==="
find domain/src -type f -name "*.java" -o -name "*.kt" | head -20Repository: samhap-soft/kokomen-payment
Length of output: 1930
🏁 Script executed:
#!/bin/bash
echo "=== Common module structure and content ==="
if [ -d "common/src/main" ]; then
find common/src/main -type f -name "*.java" -o -name "*.kt" | head -20
fi
echo ""
echo "=== Checking all imports in domain module files ==="
find domain/src/main -type f \( -name "*.java" -o -name "*.kt" \) -exec echo "File: {}" \; -exec head -30 {} \; | head -100
echo ""
echo "=== Looking for any reference to 'common' in domain ==="
rg -r "common" domain/src/main --no-filename 2>/dev/null || echo "No references found"
echo ""
echo "=== Checking domain BaseEntity class ==="
cat domain/src/main/java/com/samhap/kokomen/global/domain/BaseEntity.javaRepository: samhap-soft/kokomen-payment
Length of output: 5215
🏁 Script executed:
#!/bin/bash
echo "=== Full TosspaymentsPayment class to check public API signatures ==="
cat domain/src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.java
echo ""
echo "=== Checking TosspaymentsPaymentResult class ==="
cat domain/src/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPaymentResult.java
echo ""
echo "=== Checking repository interfaces ==="
cat domain/src/main/java/com/samhap/kokomen/payment/repository/TosspaymentsPaymentRepository.javaRepository: samhap-soft/kokomen-payment
Length of output: 8929
api 대신 implementation 사용으로 인한 API 노출 문제
domain 모듈의 TosspaymentsPayment.validateTosspaymentsResult() 메서드는 common 모듈의 InternalServerErrorException을 throws합니다. 이는 public API의 일부이므로, domain에 의존하는 모듈들이 이 예외 타입에 접근할 수 있어야 합니다.
현재 implementation으로 선언했기 때문에 common 모듈 타입이 transitively 노출되지 않습니다. 다행히 모든 소비 모듈(internal, external, api)이 이미 common을 직접 의존하고 있어 실제 문제는 없지만, 의존성 구조 관점에서는 api를 사용하는 것이 더 적절합니다.
🤖 Prompt for AI Agents
In `@domain/build.gradle` at line 2, Change the dependency declaration in the
domain module from an implementation to an api dependency so types from common
are exposed to domain consumers: replace the current implementation
project(':common') in domain's build.gradle with an api configuration; this
ensures the domain method TosspaymentsPayment.validateTosspaymentsResult() can
legitimately throw and expose common.InternalServerErrorException to modules
depending on domain.
| if (e.getCause() instanceof InvalidFormatException invalidFormatException) { | ||
| String fieldName = invalidFormatException.getPath().get(0).getFieldName(); | ||
| String invalidValue = String.valueOf(invalidFormatException.getValue()); | ||
| message = String.format( | ||
| "JSON 파싱 오류: '%s' 필드에 유효하지 않은 값이 전달되었습니다. (전달된 값: '%s')", | ||
| fieldName, | ||
| invalidValue | ||
| ); | ||
| log.warn("HttpMessageNotReadableException :: fieldName: {}, invalidValue: {}", fieldName, invalidValue); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorResponse(ExternalErrorMessage.JSON_PARSE_ERROR.getMessage())); | ||
| } |
There was a problem hiding this comment.
invalidFormatException.getPath()가 비어있을 경우 IndexOutOfBoundsException 발생 가능.
getPath().get(0)은 path 리스트가 비어있을 때 IndexOutOfBoundsException을 발생시킵니다. 방어적으로 빈 리스트 체크를 추가하는 것을 권장합니다.
🛡️ 수정 제안
if (e.getCause() instanceof InvalidFormatException invalidFormatException) {
- String fieldName = invalidFormatException.getPath().get(0).getFieldName();
+ var path = invalidFormatException.getPath();
+ String fieldName = path.isEmpty() ? "unknown" : path.get(0).getFieldName();
String invalidValue = String.valueOf(invalidFormatException.getValue());🤖 Prompt for AI Agents
In
`@external/src/main/java/com/samhap/kokomen/global/exception/GlobalExceptionHandler.java`
around lines 56 - 62, In GlobalExceptionHandler where you handle
HttpMessageNotReadableException and cast e.getCause() to InvalidFormatException
(invalidFormatException), guard against an empty path before calling
invalidFormatException.getPath().get(0) by checking
invalidFormatException.getPath() != null &&
!invalidFormatException.getPath().isEmpty(); if empty, use a safe fallback
(e.g., "unknown" or skip fieldName/invalidValue extraction) and still log and
return the same ErrorResponse; update the logic around fieldName and
invalidValue extraction in that block to avoid IndexOutOfBoundsException.
internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java
Show resolved
Hide resolved
internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java`:
- Around line 237-277: Cancel flow lacks the catch-all exception handler present
in confirmPayment, so add a generic Exception catch in
PaymentFacadeService.cancelPayment that mirrors confirmPayment's behavior: set
payment status to NEED_CANCEL (use the same NEED_CANCEL enum/constant) and throw
an InternalServerErrorException with the same error message/handling used for
unexpected errors in confirmPayment; then add a unit test (e.g.,
결제_취소_시_예상치_못한_예외가_발생하면_InternalServerErrorException을_던진다) in
PaymentFacadeServiceTest that makes tosspaymentsClient.cancelPayment throw a
RuntimeException and asserts InternalServerErrorException is thrown and the
payment status transitions to NEED_CANCEL.
🧹 Nitpick comments (3)
internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java (2)
89-99:handleConfirmServerError에서getResponseBodyAs반환값 null 가능성Line 92의
e.getResponseBodyAs(TosspaymentsPaymentResponse.class)가null을 반환하면 Line 93에서 NPE가 발생합니다. 현재catch (Exception parseException)블록으로 잡히기 때문에 기능적으로는 안전하지만, 파싱 실패가 아닌 NPE로 처리되는 것은 의도가 불명확합니다.♻️ 명시적 null 체크 추가 제안
try { TosspaymentsPaymentResponse tosspaymentsConfirmResponse = e.getResponseBodyAs(TosspaymentsPaymentResponse.class); + if (tosspaymentsConfirmResponse == null) { + throw new IllegalStateException("5xx 응답 본문이 비어 있습니다."); + } TosspaymentsPaymentResult tosspaymentsPaymentResult = tosspaymentsConfirmResponse.toTosspaymentsPaymentResult(tosspaymentsPayment);
117-137:cancelPayment의 400 에러 처리가confirmPayment와 불일치합니다.
confirmPayment에서는TosspaymentsInternalServerErrorCode.contains(code)로 서버 원인 400과 클라이언트 원인 400을 구분하여, 서버 원인이면InternalServerErrorException, 클라이언트 원인이면BadRequestException을 던집니다. 반면cancelPayment에서는 모든 non-nullFailure를BadRequestException으로 처리합니다.취소 시에도
INVALID_API_KEY같은 서버 원인 에러 코드가 반환될 수 있으므로,confirmPayment와 동일한 분기 로직을 적용하거나 공통 헬퍼로 추출하는 것을 권장합니다.♻️ 공통 헬퍼 추출 제안
} catch (HttpClientErrorException e) { Failure failure = e.getResponseBodyAs(Failure.class); if (failure == null) { log.error("결제 취소 실패(400) - 응답 파싱 실패, paymentKey: {}", request.paymentKey(), e); throw new InternalServerErrorException(PaymentServiceErrorMessage.CANCEL_SERVER_ERROR.getMessage(), e); } - log.error("결제 취소 실패(400) - paymentKey: {}, code: {}, message: {}", request.paymentKey(), failure.code(), failure.message()); - throw new BadRequestException(failure.message(), e); + if (TosspaymentsInternalServerErrorCode.contains(failure.code())) { + log.error("결제 취소 실패(서버 원인 400) - paymentKey: {}, code: {}, message: {}", request.paymentKey(), failure.code(), failure.message()); + throw new InternalServerErrorException(PaymentServiceErrorMessage.CANCEL_SERVER_ERROR.getMessage(), e); + } + log.info("결제 취소 실패(클라이언트 원인 400) - paymentKey: {}, code: {}, message: {}", request.paymentKey(), failure.code(), failure.message()); + throw new BadRequestException(failure.message(), e);또한, Line 128의 클라이언트 원인 400 에러 로그 레벨이
error로 되어 있습니다.confirmPayment에서는 클라이언트 원인 400을log.info(Line 84)로 기록하고 있으므로, 일관성을 위해 로그 레벨도 통일하는 것이 좋습니다.internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java (1)
198-235: 취소 성공 테스트의 응답 객체 구성을 헬퍼로 추출하면 가독성이 향상됩니다.
TosspaymentsCancel과TosspaymentsPaymentResponse를 인라인으로 생성하고 있어 테스트 의도 파악이 어렵습니다. 승인 테스트처럼createCancelResponse()같은 헬퍼 메서드로 추출하면 가독성과 유지보수성이 향상됩니다.
| @Test | ||
| void 결제_취소_시_400_에러가_발생하면_BadRequestException을_던진다() { | ||
| HttpClientErrorException clientError = mock(HttpClientErrorException.class); | ||
| when(clientError.getResponseBodyAs(Failure.class)) | ||
| .thenReturn(new Failure("ALREADY_CANCELED_PAYMENT", "이미 취소된 결제입니다.")); | ||
| when(tosspaymentsClient.cancelPayment(any(), any())).thenThrow(clientError); | ||
|
|
||
| assertThatThrownBy(() -> paymentFacadeService.cancelPayment(new CancelRequest("payment_key", "단순 변심"))) | ||
| .isInstanceOf(BadRequestException.class); | ||
| } | ||
|
|
||
| @Test | ||
| void 결제_취소_시_400_에러_응답_파싱에_실패하면_InternalServerErrorException을_던진다() { | ||
| HttpClientErrorException clientError = mock(HttpClientErrorException.class); | ||
| when(clientError.getResponseBodyAs(Failure.class)).thenReturn(null); | ||
| when(tosspaymentsClient.cancelPayment(any(), any())).thenThrow(clientError); | ||
|
|
||
| assertThatThrownBy(() -> paymentFacadeService.cancelPayment(new CancelRequest("payment_key", "단순 변심"))) | ||
| .isInstanceOf(InternalServerErrorException.class) | ||
| .hasMessage(PaymentServiceErrorMessage.CANCEL_SERVER_ERROR.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| void 결제_취소_시_5xx_에러가_발생하면_InternalServerErrorException을_던진다() { | ||
| when(tosspaymentsClient.cancelPayment(any(), any())) | ||
| .thenThrow(new HttpServerErrorException(HttpStatus.INTERNAL_SERVER_ERROR)); | ||
|
|
||
| assertThatThrownBy(() -> paymentFacadeService.cancelPayment(new CancelRequest("payment_key", "단순 변심"))) | ||
| .isInstanceOf(InternalServerErrorException.class) | ||
| .hasMessage(PaymentServiceErrorMessage.CANCEL_SERVER_ERROR.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| void 결제_취소_시_네트워크_에러가_발생하면_InternalServerErrorException을_던진다() { | ||
| when(tosspaymentsClient.cancelPayment(any(), any())) | ||
| .thenThrow(new ResourceAccessException("네트워크 오류")); | ||
|
|
||
| assertThatThrownBy(() -> paymentFacadeService.cancelPayment(new CancelRequest("payment_key", "단순 변심"))) | ||
| .isInstanceOf(InternalServerErrorException.class) | ||
| .hasMessage(PaymentServiceErrorMessage.CANCEL_NETWORK_ERROR.getMessage()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file and check its structure
find . -name "PaymentFacadeServiceTest.java" -type f
Repository: samhap-soft/kokomen-payment
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Locate the PaymentFacadeService implementation
find . -name "PaymentFacadeService.java" -type f | grep -v Test
Repository: samhap-soft/kokomen-payment
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Search for the approval flow test mentioned in the comment
rg -n "결제_승인_시_예상치_못한_예외가_발생하면" --type=java
Repository: samhap-soft/kokomen-payment
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Read the test file to see the approval flow test and full context
wc -l "./internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java"
Repository: samhap-soft/kokomen-payment
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Read the approval flow test around line 187
sed -n '180,220p' "./internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java"
Repository: samhap-soft/kokomen-payment
Length of output: 2084
🏁 Script executed:
#!/bin/bash
# Read the PaymentFacadeService implementation
wc -l "./internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java"
Repository: samhap-soft/kokomen-payment
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Read the full PaymentFacadeService implementation
cat -n "./internal/src/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.java"
Repository: samhap-soft/kokomen-payment
Length of output: 9112
🏁 Script executed:
#!/bin/bash
# Read the cancellation tests from line 230 to end
sed -n '230,292p' "./internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java"
Repository: samhap-soft/kokomen-payment
Length of output: 3262
취소 흐름에서 예상치 못한 예외 발생 시 처리가 승인 흐름과 다릅니다.
승인 흐름(confirmPayment)에는 HttpClientErrorException, HttpServerErrorException, ResourceAccessException 외의 예외가 발생하면 NEED_CANCEL 상태로 변경하는 catch-all 핸들러(43~47줄)가 있습니다. 그러나 취소 흐름(cancelPayment)은 위 세 가지 예외만 처리하고 catch-all 핸들러가 없어, 예상치 못한 RuntimeException 등의 예외는 그대로 전파됩니다.
일관성을 위해 취소 흐름에도 같은 패턴의 catch-all 핸들러를 추가하고, 이에 대한 테스트 케이스(결제_취소_시_예상치_못한_예외가_발생하면_InternalServerErrorException을_던진다 등)를 추가하는 것을 검토하세요.
🤖 Prompt for AI Agents
In
`@internal/src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java`
around lines 237 - 277, Cancel flow lacks the catch-all exception handler
present in confirmPayment, so add a generic Exception catch in
PaymentFacadeService.cancelPayment that mirrors confirmPayment's behavior: set
payment status to NEED_CANCEL (use the same NEED_CANCEL enum/constant) and throw
an InternalServerErrorException with the same error message/handling used for
unexpected errors in confirmPayment; then add a unit test (e.g.,
결제_취소_시_예상치_못한_예외가_발생하면_InternalServerErrorException을_던진다) in
PaymentFacadeServiceTest that makes tosspaymentsClient.cancelPayment throw a
RuntimeException and asserts InternalServerErrorException is thrown and the
payment status transitions to NEED_CANCEL.
1 participant
closed #14
Summary by CodeRabbit
새 기능
버그 수정
개선사항
테스트