Open
Conversation
📝 WalkthroughWalkthrough결제 작업에 사용자 기반 인증을 추가하여 인증된 사용자에서 userId를 추출하고 결제 소유권을 검증합니다. 저장소 쿼리를 관련 엔터티를 즉시 로드하도록 업데이트하고 비밀값에 대한 환경 기반 구성을 도입합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentController
participant UserRepository
participant PaymentService
participant PaymentRepository
participant Database
Client->>PaymentController: GET /payments/detail/{paymentId}<br/>(with auth token)
PaymentController->>PaymentController: Extract User from `@AuthenticationPrincipal`
PaymentController->>UserRepository: findByEmail(user.email)
UserRepository->>Database: Query user by email
Database-->>UserRepository: User record
UserRepository-->>PaymentController: User ID
PaymentController->>PaymentService: getPaymentDetail(paymentId, userId)
PaymentService->>PaymentRepository: findByIdWithDetails(paymentId)
PaymentRepository->>Database: SELECT p FROM Payment p<br/>JOIN FETCH p.booking b<br/>JOIN FETCH b.store<br/>JOIN FETCH b.user
Database-->>PaymentRepository: Payment with associations
PaymentRepository-->>PaymentService: Payment entity
PaymentService->>PaymentService: Validate ownership<br/>(payment.booking.user.id == userId)
alt Ownership Valid
PaymentService-->>PaymentController: Payment with details
PaymentController-->>Client: 200 OK + Payment JSON
else Ownership Invalid
PaymentService-->>PaymentController: PaymentException<br/>(PAYMENT_ACCESS_DENIED)
PaymentController-->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/eatsfine/eatsfine/domain/payment/service/PaymentService.java (1)
43-53:⚠️ Potential issue | 🟠 Major
requestPayment,confirmPayment,cancelPayment엔드포인트에 사용자 인증 및 소유권 검증이 누락되어 있습니다.
getPaymentList와getPaymentDetail에는@AuthenticationPrincipal과 소유권 검증이 적용되었지만, 결제 요청·승인·취소 엔드포인트는 인증 없이 호출 가능한 상태입니다:
requestPayment: 임의의bookingId로 누구나 결제를 생성할 수 있음confirmPayment:orderId,amount,paymentKey를 알면 누구나 결제를 승인할 수 있음cancelPayment:paymentKey만 알면 누구나 결제를 취소할 수 있음이는
PaymentWebhookController가 별도로 존재하고 서명 검증을 수행하는 것과 별개로, 사용자 요청용 컨트롤러 엔드포인트에도 인증 및 소유권 검증이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/eatsfine/eatsfine/domain/payment/service/PaymentService.java` around lines 43 - 53, Add authentication and ownership checks to requestPayment, confirmPayment, and cancelPayment: require the authenticated principal (add an `@AuthenticationPrincipal` parameter, e.g., Principal or CustomUserDetails) in each method signature, then verify ownership by comparing the booking or payment owner id (use Booking booking = bookingRepository.findById(...) in requestPayment and validate booking.getMemberId()/getUserId() against principal.getId(); in confirmPayment locate the payment by orderId/paymentKey and validate its owner; in cancelPayment locate the payment by paymentKey and validate its owner) and throw the appropriate PaymentException (use the existing PaymentErrorStatus value for unauthorized access) when the principal does not match. Ensure you re-use method names requestPayment, confirmPayment, cancelPayment and repository lookup methods to locate the entities before proceeding.
🤖 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/eatsfine/eatsfine/domain/payment/controller/PaymentController.java`:
- Line 31: The PaymentController currently injects and uses UserRepository
(field userRepository), which violates layering; move user-lookup logic out of
the controller by delegating to a dedicated component such as a UserService or
AuthUserResolver: create or reuse a service (e.g., UserService or
AuthUserResolver with a method resolveUserId(User user) or similar) that
encapsulates the repository call and exception handling, inject that service
into PaymentController in place of UserRepository, and update BookingController
and any other controllers using the same pattern to call the shared
resolver/service instead of referencing UserRepository directly; ensure
PaymentService handles business rules and controllers only orchestrate requests.
In `@src/main/resources/application-local.yml`:
- Around line 72-74: The YAML is missing a blank line between the top-level
sections 'api' and 'jwt', causing inconsistent spacing with other sections like
'payment' and 'cloud'; insert a single empty line between the 'api' block
(service-key: ${BIZ_API_KEY}) and the 'jwt' key in application-local.yml so all
top-level sections have the same blank-line separation for consistency.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/controller/PaymentControllerTest.java`:
- Around line 153-200: Tests only cover success paths for getPaymentList and
getPaymentDetail; add negative/auth and error-case unit tests: (1) an
unauthenticated request (call the same endpoints without `@WithMockUser`)
asserting 401/403; (2) a "user not found" case by stubbing
userRepository.findByEmail(...) to return Optional.empty() and asserting the
controller returns the expected error response/status; (3) an access-denied case
by stubbing paymentService.getPaymentDetail(paymentId, userId) to throw the
PaymentException with code _PAYMENT_ACCESS_DENIED and asserting the controller
maps it to the correct error status/body; reference the existing test methods
getPaymentList_success and getPaymentDetail_success, the
userRepository.findByEmail, paymentService.getPaymentDetail, and
PaymentException/_PAYMENT_ACCESS_DENIED when adding these new test methods.
- Around line 53-66: The tests use deprecated `@MockBean`; update the five mock
declarations in PaymentControllerTest (fields paymentService,
jwtAuthenticationFilter, customAuthenticationEntryPoint,
customAccessDeniedHandler, userRepository) to use `@MockitoBean` instead and
change the import from org.springframework.boot.test.mock.mockito.MockBean to
org.springframework.test.context.bean.override.mockito.MockitoBean; apply the
same import+annotation replacement in other test classes named (e.g.,
HealthControllerTest, InquiryControllerTest) so all Mockito-based test beans use
`@MockitoBean`.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/service/PaymentServiceTest.java`:
- Around line 39-217: Add unit tests in PaymentServiceTest covering ownership
checks and list retrieval: (1) a success case for
PaymentService.getPaymentDetail where the requesting user's id matches the
payment's owner and the method returns expected DTO; (2) an access-denied case
for getPaymentDetail where paymentRepository.findById(...) returns a Payment
owned by a different user and assert that calling
paymentService.getPaymentDetail(...) throws PaymentException with code
PaymentErrorStatus._PAYMENT_ACCESS_DENIED and that the payment status remains
unchanged; and (3) a normal getPaymentList case that stubs
paymentRepository.findByBookingMemberId(...) or the repository method used by
PaymentService.getPaymentList to return a page/list of payments and assert the
returned list/DTO content matches expectations; use the existing test patterns
(given(...).willReturn(...), assertThat(...), assertThatThrownBy(...),
verify(...)) and the same helper symbols PaymentService, PaymentRepository,
PaymentResponseDTO, PaymentErrorStatus._PAYMENT_ACCESS_DENIED, and
PaymentServiceTest to locate where to add these tests.
- Around line 129-142: Remove the long inline Korean comment after the
TossPaymentResponse constructor call in PaymentServiceTest and replace it by
extracting a helper/builder method (e.g., createTossPaymentResponse(...) or a
TossPaymentResponseTestBuilder) to construct the test response; update the test
to call that helper (referencing TossPaymentResponse and PaymentServiceTest) so
the test body stays concise and all construction details live in the new helper
method.
---
Outside diff comments:
In
`@src/main/java/com/eatsfine/eatsfine/domain/payment/service/PaymentService.java`:
- Around line 43-53: Add authentication and ownership checks to requestPayment,
confirmPayment, and cancelPayment: require the authenticated principal (add an
`@AuthenticationPrincipal` parameter, e.g., Principal or CustomUserDetails) in
each method signature, then verify ownership by comparing the booking or payment
owner id (use Booking booking = bookingRepository.findById(...) in
requestPayment and validate booking.getMemberId()/getUserId() against
principal.getId(); in confirmPayment locate the payment by orderId/paymentKey
and validate its owner; in cancelPayment locate the payment by paymentKey and
validate its owner) and throw the appropriate PaymentException (use the existing
PaymentErrorStatus value for unauthorized access) when the principal does not
match. Ensure you re-use method names requestPayment, confirmPayment,
cancelPayment and repository lookup methods to locate the entities before
proceeding.
| public class PaymentController { | ||
|
|
||
| private final PaymentService paymentService; | ||
| private final UserRepository userRepository; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Controller에서 UserRepository를 직접 참조하는 것은 계층 구조 위반입니다.
Controller가 Repository를 직접 의존하면 계층 간 결합도가 높아지고 단일 책임 원칙(SRP)에 어긋납니다. 사용자 조회 로직은 별도의 UserService로 위임하거나, PaymentService에서 인증 사용자 해석을 담당하는 것이 바람직합니다.
다만, 기존 BookingController 등에서 동일한 패턴을 사용하고 있다면, 프로젝트 전체적으로 공통 유틸리티(예: AuthUserResolver)를 추출하는 리팩토링을 고려해 주세요.
As per coding guidelines, "Controller, Service, Repository 등 계층 구조가 올바르게 나뉘어 있는지" 확인 필요.
♻️ 공통 유틸리티 추출 제안
별도의 컴포넌트로 사용자 ID 해석 로직을 추출:
`@Component`
`@RequiredArgsConstructor`
public class AuthUserResolver {
private final UserRepository userRepository;
public Long resolveUserId(User user) {
String email = user.getUsername();
return userRepository.findByEmail(email)
.orElseThrow(() -> new UserException(UserErrorStatus.MEMBER_NOT_FOUND))
.getId();
}
}또는 HandlerMethodArgumentResolver를 구현하여 @CurrentUserId Long userId와 같은 커스텀 어노테이션으로 자동 해석하는 방법도 있습니다.
Also applies to: 75-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/eatsfine/eatsfine/domain/payment/controller/PaymentController.java`
at line 31, The PaymentController currently injects and uses UserRepository
(field userRepository), which violates layering; move user-lookup logic out of
the controller by delegating to a dedicated component such as a UserService or
AuthUserResolver: create or reuse a service (e.g., UserService or
AuthUserResolver with a method resolveUserId(User user) or similar) that
encapsulates the repository call and exception handling, inject that service
into PaymentController in place of UserRepository, and update BookingController
and any other controllers using the same pattern to call the shared
resolver/service instead of referencing UserRepository directly; ensure
PaymentService handles business rules and controllers only orchestrate requests.
Comment on lines
+72
to
74
| api: | ||
| service-key: ${BIZ_API_KEY} | ||
| jwt: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
api와 jwt 섹션 사이에 빈 줄 누락.
다른 최상위 섹션(payment, cloud 등) 사이에는 빈 줄이 있지만, api(Line 72)와 jwt(Line 74) 사이에는 빈 줄이 없어 일관성이 떨어집니다.
제안
api:
service-key: ${BIZ_API_KEY}
+
jwt:
secret: ${SECRET_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
| api: | |
| service-key: ${BIZ_API_KEY} | |
| jwt: | |
| api: | |
| service-key: ${BIZ_API_KEY} | |
| jwt: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application-local.yml` around lines 72 - 74, The YAML is
missing a blank line between the top-level sections 'api' and 'jwt', causing
inconsistent spacing with other sections like 'payment' and 'cloud'; insert a
single empty line between the 'api' block (service-key: ${BIZ_API_KEY}) and the
'jwt' key in application-local.yml so all top-level sections have the same
blank-line separation for consistency.
Comment on lines
53
to
66
| @MockBean | ||
| private PaymentService paymentService; | ||
|
|
||
| @MockBean | ||
| private JwtAuthenticationFilter jwtAuthenticationFilter; | ||
|
|
||
| @MockBean | ||
| private CustomAuthenticationEntryPoint customAuthenticationEntryPoint; | ||
|
|
||
| @MockBean | ||
| private CustomAccessDeniedHandler customAccessDeniedHandler; | ||
|
|
||
| @MockBean | ||
| private UserRepository userRepository; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Spring Boot version in build files
echo "=== Checking for Spring Boot version in gradle build files ==="
find . -type f -name "build.gradle*" | head -5 | xargs cat 2>/dev/null | grep -i "spring.boot\|springBootVersion\|org.springframework.boot" | head -10
echo ""
echo "=== Checking for Spring Boot version in Maven pom files ==="
find . -type f -name "pom.xml" | head -5 | xargs cat 2>/dev/null | grep -A 2 "spring-boot" | head -15
echo ""
echo "=== Checking usage of `@MockBean` in test files ==="
rg '@MockBean' --type java -g '*Test*.java' -c | head -10
echo ""
echo "=== Checking usage of `@MockitoBean` in test files ==="
rg '@MockitoBean' --type java -g '*Test*.java' -c | head -10Repository: Eatsfine/BE
Length of output: 1193
🌐 Web query:
Spring Boot 3.4 @MockBeandeprecated@MockitoBean replacement
💡 Result:
In Spring Boot 3.4, @MockBean / @MockBeans are deprecated. Replace them with Spring Framework’s @MockitoBean (and @SpyBean with @MockitoSpyBean). (github.com)
1) Field-level mock (most common)
Before
import org.springframework.boot.test.mock.mockito.MockBean;
`@MockBean`
MyService myService;After
import org.springframework.test.context.bean.override.mockito.MockitoBean;
`@MockitoBean`
MyService myService;2) Type-level / multiple mocks (replacement for @MockBean({ ... }) and many @MockBeans)
Use types = { ... } (or repeat @MockitoBean since it’s @Repeatable):
import org.springframework.test.context.bean.override.mockito.MockitoBean;
`@MockitoBean`(types = { Foo.class, Bar.class })
`@SpringBootTest`
class MyTest { }Important migration note
Spring Boot calls out that Spring Framework’s replacements are not identical to Boot’s old behavior (e.g., @MockitoBean is not supported on @Configuration classes), so you may need to move mocks onto test-class fields (or a shared base test class). (github.com)
Citations:
- 1: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.4-Release-Notes?utm_source=openai
- 2: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/test/context/bean/override/mockito/MockitoBean.html?utm_source=openai
- 3: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/test/context/bean/override/mockito/MockitoBean.html?utm_source=openai
- 4: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.4-Release-Notes?utm_source=openai
@MockBean은 Spring Boot 3.4+에서 deprecated되었으므로 @MockitoBean으로 마이그레이션이 필요합니다.
프로젝트가 Spring Boot 3.4.1을 사용 중이며, @MockBean은 공식 문서에서 deprecated 처리되었습니다. 해당 파일의 다섯 개 MockBean 선언을 다음과 같이 마이그레이션하세요:
- import 변경:
org.springframework.boot.test.mock.mockito.MockBean→org.springframework.test.context.bean.override.mockito.MockitoBean - 어노테이션 변경:
@MockBean→@MockitoBean
마찬가지로 프로젝트의 다른 테스트 파일(HealthControllerTest, InquiryControllerTest)도 동일하게 업데이트하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/controller/PaymentControllerTest.java`
around lines 53 - 66, The tests use deprecated `@MockBean`; update the five mock
declarations in PaymentControllerTest (fields paymentService,
jwtAuthenticationFilter, customAuthenticationEntryPoint,
customAccessDeniedHandler, userRepository) to use `@MockitoBean` instead and
change the import from org.springframework.boot.test.mock.mockito.MockBean to
org.springframework.test.context.bean.override.mockito.MockitoBean; apply the
same import+annotation replacement in other test classes named (e.g.,
HealthControllerTest, InquiryControllerTest) so all Mockito-based test beans use
`@MockitoBean`.
Comment on lines
153
to
200
| @Test | ||
| @DisplayName("결제 내역 조회 성공") | ||
| void getPaymentList_success() throws Exception { | ||
| // given | ||
| PaymentResponseDTO.PaginationDTO pagination = new PaymentResponseDTO.PaginationDTO(1, 1, 1L); | ||
| PaymentResponseDTO.PaymentHistoryResultDTO history = new PaymentResponseDTO.PaymentHistoryResultDTO( | ||
| 1L, 1L, "Store Name", BigDecimal.valueOf(10000), "DEPOSIT", "CARD", "TOSS", "COMPLETED", | ||
| LocalDateTime.now()); | ||
| PaymentResponseDTO.PaymentListResponseDTO response = new PaymentResponseDTO.PaymentListResponseDTO( | ||
| Collections.singletonList(history), pagination); | ||
|
|
||
| User user = User.builder().id(1L).email("user").role(Role.ROLE_CUSTOMER).build(); | ||
| given(userRepository.findByEmail("user")).willReturn(Optional.of(user)); | ||
|
|
||
| given(paymentService.getPaymentList(eq(1L), any(Integer.class), any(Integer.class), any())) | ||
| .willReturn(response); | ||
|
|
||
| // when & then | ||
| mockMvc.perform(get("/api/v1/payments") | ||
| .param("page", "1") | ||
| .param("limit", "10") | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.isSuccess").value(true)) | ||
| .andExpect(jsonPath("$.result.payments[0].storeName").value("Store Name")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("결제 상세 조회 성공") | ||
| void getPaymentDetail_success() throws Exception { | ||
| // given | ||
| Long paymentId = 1L; | ||
| PaymentResponseDTO.PaymentDetailResultDTO response = new PaymentResponseDTO.PaymentDetailResultDTO( | ||
| paymentId, 1L, "Store Name", "CARD", "TOSS", BigDecimal.valueOf(10000), "DEPOSIT", | ||
| "COMPLETED", LocalDateTime.now(), LocalDateTime.now(), "http://receipt.url", null); | ||
|
|
||
| User user = User.builder().id(1L).email("user").role(Role.ROLE_CUSTOMER).build(); | ||
| given(userRepository.findByEmail("user")).willReturn(Optional.of(user)); | ||
|
|
||
| given(paymentService.getPaymentDetail(eq(paymentId), eq(1L))).willReturn(response); | ||
|
|
||
| // when & then | ||
| mockMvc.perform(get("/api/v1/payments/{paymentId}", paymentId) | ||
| .contentType(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.isSuccess").value(true)) | ||
| .andExpect(jsonPath("$.result.paymentId").value(paymentId)); | ||
| } |
There was a problem hiding this comment.
인증 실패 및 에러 케이스 테스트가 누락되었습니다.
getPaymentList와 getPaymentDetail의 성공 케이스만 테스트되어 있습니다. 보안 강화가 PR의 핵심 목표인 만큼, 다음 케이스를 추가해 주세요:
- 인증되지 않은 요청:
@WithMockUser없이 호출 시 401/403 응답 확인 - 사용자 미조회:
userRepository.findByEmail()이Optional.empty()를 반환할 때 예외 처리 확인 - 다른 사용자의 결제 상세 접근:
paymentService.getPaymentDetail()에서PaymentException(_PAYMENT_ACCESS_DENIED) 발생 시 적절한 에러 응답 확인
As per coding guidelines, "경계값 테스트, 예외 케이스 테스트가 포함되어 있는지" 확인 필요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/controller/PaymentControllerTest.java`
around lines 153 - 200, Tests only cover success paths for getPaymentList and
getPaymentDetail; add negative/auth and error-case unit tests: (1) an
unauthenticated request (call the same endpoints without `@WithMockUser`)
asserting 401/403; (2) a "user not found" case by stubbing
userRepository.findByEmail(...) to return Optional.empty() and asserting the
controller returns the expected error response/status; (3) an access-denied case
by stubbing paymentService.getPaymentDetail(paymentId, userId) to throw the
PaymentException with code _PAYMENT_ACCESS_DENIED and asserting the controller
maps it to the correct error status/body; reference the existing test methods
getPaymentList_success and getPaymentDetail_success, the
userRepository.findByEmail, paymentService.getPaymentDetail, and
PaymentException/_PAYMENT_ACCESS_DENIED when adding these new test methods.
Comment on lines
39
to
217
| @ExtendWith(MockitoExtension.class) | ||
| class PaymentServiceTest { | ||
|
|
||
| @InjectMocks | ||
| private PaymentService paymentService; | ||
|
|
||
| @Mock | ||
| private PaymentRepository paymentRepository; | ||
|
|
||
| @Mock | ||
| private BookingRepository bookingRepository; | ||
|
|
||
| @Mock | ||
| private TossPaymentService tossPaymentService; | ||
|
|
||
| @Test | ||
| @DisplayName("결제 요청 성공") | ||
| void requestPayment_success() { | ||
| // given | ||
| Long bookingId = 1L; | ||
| PaymentRequestDTO.RequestPaymentDTO request = new PaymentRequestDTO.RequestPaymentDTO(bookingId); | ||
|
|
||
| Booking booking = Booking.builder() | ||
| .id(bookingId) | ||
| .build(); | ||
| ReflectionTestUtils.setField(booking, "depositAmount", BigDecimal.valueOf(10000)); | ||
|
|
||
| Payment payment = Payment.builder() | ||
| .id(1L) | ||
| .booking(booking) | ||
| .amount(BigDecimal.valueOf(10000)) | ||
| .orderId("generated-order-id") | ||
| .paymentStatus(PaymentStatus.PENDING) | ||
| .requestedAt(LocalDateTime.now()) | ||
| .build(); | ||
|
|
||
| given(bookingRepository.findById(bookingId)).willReturn(Optional.of(booking)); | ||
| given(paymentRepository.save(any(Payment.class))).willReturn(payment); | ||
|
|
||
| // when | ||
| PaymentResponseDTO.PaymentRequestResultDTO response = paymentService.requestPayment(request); | ||
|
|
||
| // then | ||
| assertThat(response.amount()).isEqualTo(BigDecimal.valueOf(10000)); | ||
| assertThat(response.orderId()).isEqualTo("generated-order-id"); | ||
| verify(paymentRepository, times(1)).save(any(Payment.class)); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("결제 요청 실패 - 예약 없음") | ||
| void requestPayment_fail_bookingNotFound() { | ||
| // given | ||
| PaymentRequestDTO.RequestPaymentDTO request = new PaymentRequestDTO.RequestPaymentDTO(999L); | ||
| given(bookingRepository.findById(999L)).willReturn(Optional.empty()); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> paymentService.requestPayment(request)) | ||
| .isInstanceOf(PaymentException.class) | ||
| .extracting("code") | ||
| .isEqualTo(PaymentErrorStatus._BOOKING_NOT_FOUND); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("결제 승인 성공") | ||
| void confirmPayment_success() { | ||
| // given | ||
| String orderId = "order-id-123"; | ||
| String paymentKey = "payment-key-123"; | ||
| BigDecimal amount = BigDecimal.valueOf(10000); | ||
| PaymentConfirmDTO request = PaymentConfirmDTO.builder() | ||
| .orderId(orderId) | ||
| .amount(amount) | ||
| .paymentKey(paymentKey) | ||
| .build(); | ||
|
|
||
| Booking booking = Booking.builder().id(1L).build(); | ||
| Payment payment = Payment.builder() | ||
| .id(1L) | ||
| .booking(booking) | ||
| .orderId(orderId) | ||
| .amount(amount) | ||
| .paymentStatus(PaymentStatus.PENDING) | ||
| .build(); | ||
|
|
||
| TossPaymentResponse.EasyPay easyPay = new TossPaymentResponse.EasyPay("토스페이", 10000, 0); | ||
| TossPaymentResponse tossResponse = new TossPaymentResponse( | ||
| paymentKey, "NORMAL", orderId, "orderName", "mId", "KRW", "CARD", | ||
| 10000, 10000, "DONE", | ||
| java.time.OffsetDateTime.now(), java.time.OffsetDateTime.now(), | ||
| false, null, 10000, 0, | ||
| easyPay, new TossPaymentResponse.Receipt("http://receipt.url"));// TossPaymentResponse | ||
| // record 생성자가 많아서 | ||
| // 필드에 맞게 넣어줌 (가정) | ||
| // 실제 record 구조에 따라 맞춰야 | ||
| // 함. 위 내용은 예시. | ||
| // TossPaymentResponse가 | ||
| // record이므로 생성자 | ||
| // 파라미터 순서 중요. | ||
| // 여기서는 Mocking을 하거나, | ||
| // 필드가 많으면 빌더나 생성자를 | ||
| // 확인해야 함. | ||
| // TossPaymentService가 | ||
| // Mock이므로 response | ||
| // 리턴값만 잘 맞춰주면 됨. | ||
|
|
||
| given(paymentRepository.findByOrderId(orderId)).willReturn(Optional.of(payment)); | ||
| given(tossPaymentService.confirm(any(PaymentConfirmDTO.class))).willReturn(tossResponse); | ||
|
|
||
| // when | ||
| PaymentResponseDTO.PaymentSuccessResultDTO response = paymentService.confirmPayment(request); | ||
|
|
||
| // then | ||
| assertThat(response.status()).isEqualTo(PaymentStatus.COMPLETED.name()); | ||
| assertThat(payment.getPaymentStatus()).isEqualTo(PaymentStatus.COMPLETED); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("결제 승인 실패 - 금액 불일치") | ||
| void confirmPayment_fail_invalidAmount() { | ||
| // given | ||
| String orderId = "order-id-123"; | ||
| BigDecimal originalAmount = BigDecimal.valueOf(10000); | ||
| BigDecimal requestAmount = BigDecimal.valueOf(5000); // Mismatch | ||
|
|
||
| PaymentConfirmDTO request = PaymentConfirmDTO.builder() | ||
| .orderId(orderId) | ||
| .amount(requestAmount) | ||
| .paymentKey("key") | ||
| .build(); | ||
|
|
||
| Payment payment = Payment.builder() | ||
| .id(1L) | ||
| .orderId(orderId) | ||
| .amount(originalAmount) | ||
| .paymentStatus(PaymentStatus.PENDING) | ||
| .build(); | ||
|
|
||
| given(paymentRepository.findByOrderId(orderId)).willReturn(Optional.of(payment)); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(() -> paymentService.confirmPayment(request)) | ||
| .isInstanceOf(PaymentException.class) | ||
| .extracting("code") | ||
| .isEqualTo(PaymentErrorStatus._PAYMENT_INVALID_AMOUNT); | ||
|
|
||
| assertThat(payment.getPaymentStatus()).isEqualTo(PaymentStatus.FAILED); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("결제 취소 성공") | ||
| void cancelPayment_success() { | ||
| // given | ||
| String paymentKey = "payment-key-123"; | ||
| PaymentRequestDTO.CancelPaymentDTO request = new PaymentRequestDTO.CancelPaymentDTO("단순 변심"); | ||
|
|
||
| Payment payment = Payment.builder() | ||
| .id(1L) | ||
| .paymentKey(paymentKey) | ||
| .orderId("order-id-123") | ||
| .paymentStatus(PaymentStatus.COMPLETED) | ||
| .build(); | ||
|
|
||
| TossPaymentResponse tossResponse = new TossPaymentResponse( | ||
| paymentKey, "NORMAL", "order-id-123", "orderName", "mId", "KRW", "CARD", | ||
| 10000, 0, "CANCELED", | ||
| java.time.OffsetDateTime.now(), java.time.OffsetDateTime.now(), | ||
| false, null, 10000, 0, | ||
| null, null); | ||
| given(paymentRepository.findByPaymentKey(paymentKey)).willReturn(Optional.of(payment)); | ||
| given(tossPaymentService.cancel(eq(paymentKey), any(PaymentRequestDTO.CancelPaymentDTO.class))) | ||
| .willReturn(tossResponse); | ||
|
|
||
| // when | ||
| PaymentResponseDTO.CancelPaymentResultDTO response = paymentService.cancelPayment(paymentKey, request); | ||
|
|
||
| // then | ||
| assertThat(response.status()).isEqualTo("REFUNDED"); | ||
| assertThat(payment.getPaymentStatus()).isEqualTo(PaymentStatus.REFUNDED); | ||
| } |
There was a problem hiding this comment.
getPaymentDetail의 소유권 검증 테스트가 누락되었습니다.
이 PR의 핵심 보안 변경사항인 getPaymentDetail의 소유권 검증(_PAYMENT_ACCESS_DENIED) 및 정상 조회 케이스에 대한 테스트가 없습니다. getPaymentList에 대한 테스트도 누락되어 있습니다. 최소한 다음 케이스를 추가해 주세요:
getPaymentDetail— 본인의 결제 조회 성공getPaymentDetail— 다른 사용자의 결제 접근 시_PAYMENT_ACCESS_DENIED예외 발생getPaymentList— 정상 조회
As per coding guidelines, "단위 테스트가 충분히 작성되었는지, 핵심 로직의 검증이 누락되지 않았는지" 및 "경계값 테스트, 예외 케이스 테스트가 포함되어 있는지" 확인 필요.
해당 테스트 코드를 생성해 드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/service/PaymentServiceTest.java`
around lines 39 - 217, Add unit tests in PaymentServiceTest covering ownership
checks and list retrieval: (1) a success case for
PaymentService.getPaymentDetail where the requesting user's id matches the
payment's owner and the method returns expected DTO; (2) an access-denied case
for getPaymentDetail where paymentRepository.findById(...) returns a Payment
owned by a different user and assert that calling
paymentService.getPaymentDetail(...) throws PaymentException with code
PaymentErrorStatus._PAYMENT_ACCESS_DENIED and that the payment status remains
unchanged; and (3) a normal getPaymentList case that stubs
paymentRepository.findByBookingMemberId(...) or the repository method used by
PaymentService.getPaymentList to return a page/list of payments and assert the
returned list/DTO content matches expectations; use the existing test patterns
(given(...).willReturn(...), assertThat(...), assertThatThrownBy(...),
verify(...)) and the same helper symbols PaymentService, PaymentRepository,
PaymentResponseDTO, PaymentErrorStatus._PAYMENT_ACCESS_DENIED, and
PaymentServiceTest to locate where to add these tests.
Comment on lines
+129
to
+142
| easyPay, new TossPaymentResponse.Receipt("http://receipt.url"));// TossPaymentResponse | ||
| // record 생성자가 많아서 | ||
| // 필드에 맞게 넣어줌 (가정) | ||
| // 실제 record 구조에 따라 맞춰야 | ||
| // 함. 위 내용은 예시. | ||
| // TossPaymentResponse가 | ||
| // record이므로 생성자 | ||
| // 파라미터 순서 중요. | ||
| // 여기서는 Mocking을 하거나, | ||
| // 필드가 많으면 빌더나 생성자를 | ||
| // 확인해야 함. | ||
| // TossPaymentService가 | ||
| // Mock이므로 response | ||
| // 리턴값만 잘 맞춰주면 됨. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
불필요한 인라인 주석을 정리해 주세요.
TossPaymentResponse 생성자 호출 뒤에 붙은 긴 한국어 주석(Lines 129-142)은 코드 가독성을 저해합니다. 테스트 코드에서 생성자 파라미터가 많다면, 헬퍼 메서드나 테스트 빌더를 추출하는 것이 더 효과적입니다.
♻️ 헬퍼 메서드 추출 제안
+ private TossPaymentResponse createSuccessTossResponse(String paymentKey, String orderId) {
+ TossPaymentResponse.EasyPay easyPay = new TossPaymentResponse.EasyPay("토스페이", 10000, 0);
+ return new TossPaymentResponse(
+ paymentKey, "NORMAL", orderId, "orderName", "mId", "KRW", "CARD",
+ 10000, 10000, "DONE",
+ java.time.OffsetDateTime.now(), java.time.OffsetDateTime.now(),
+ false, null, 10000, 0,
+ easyPay, new TossPaymentResponse.Receipt("http://receipt.url"));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/eatsfine/eatsfine/domain/payment/service/PaymentServiceTest.java`
around lines 129 - 142, Remove the long inline Korean comment after the
TossPaymentResponse constructor call in PaymentServiceTest and replace it by
extracting a helper/builder method (e.g., createTossPaymentResponse(...) or a
TossPaymentResponseTestBuilder) to construct the test response; update the test
to call that helper (referencing TossPaymentResponse and PaymentServiceTest) so
the test body stays concise and all construction details live in the new helper
method.
- Service 계층으로 User 조회 로직 이관 및 Controller 의존성 제거 - PaymentService 단위 테스트(성공/실패/예외 케이스) 추가 - 결제 상세 조회 시, 예약자뿐만 아니라 상점 주인도 접근 가능하도록 권한 로직 수정 - Spring Boot 3.4 마이그레이션: @MockBean -> @MockitoBean 교체 (Deprecated API 대응)
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.
💡 작업 개요
✅ 작업 내용
🧪 테스트 내용
📝 기타 참고 사항
Summary by CodeRabbit
릴리스 노트
새 기능
개선 사항
테스트