Conversation
gkdudans
requested review from
NamYeonW00,
choigpt,
doehy,
geleego and
ghkddlscks19
Walkthrough결제 실패 보고 엔드포인트 추가와 결제 엔티티/레포 변경(결제키 기반 락·멱등성), 정산 로직을 WalletService로 위임해 총액 계산·트랜잭션 흐름 변경, 클럽 목록 조회 API를 ClubController에서 SearchController로 이동 및 멤버 수 반환 쿼리 추가. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentController
participant PaymentService
participant PaymentRepository
participant WalletService
Client->>PaymentController: POST /payments/fail (ConfirmTossPayRequest)
PaymentController->>PaymentService: reportFail(req)
PaymentService->>PaymentRepository: existsByTossPaymentKey(req.tossPaymentKey)
alt exists
PaymentService-->>PaymentController: return (idempotent no-op)
else
PaymentService->>WalletService: createFailedWalletTransactions(walletId, leaderWalletId, amount, userSettlement)
PaymentService->>PaymentRepository: save Payment(status=CANCELED, tossPaymentKey...)
end
PaymentController-->>Client: 200 OK
sequenceDiagram
participant SettlementService
participant WalletService
participant UserSettlement
participant NotificationService
SettlementService->>SettlementService: total = (userCount - 1) * cost
loop per member
SettlementService->>WalletService: createSuccessfulWalletTransactions(walletId, leaderWalletId, amount, userSettlement)
WalletService->>UserSettlement: create Transfer & mark COMPLETED
SettlementService->>NotificationService: send completion notification
end
alt failure
SettlementService->>WalletService: createFailedWalletTransactions(...)(REQUIRES_NEW)
WalletService->>UserSettlement: mark FAILED
SettlementService-->>SettlementService: throw SETTLEMENT_PROCESS_FAILED
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java (1)
4-4: 불필요한 import 제거
jakarta.validation.constraints.NotBlank는 사용되지 않습니다. 제거해 주세요.-import jakarta.validation.constraints.NotBlank;src/main/java/com/example/onlyone/domain/settlement/entity/UserSettlement.java (1)
47-49: 부분 업데이트 메서드 추가는 좋습니다. 파라미터 null 방지 주석/어노테이션 권장
settlementStatus는 의미상 null이 될 수 없어 보입니다. 메서드 시그니처에 @NotNull을 추가해 조기 검증을 권장합니다.- public void updateStatus(SettlementStatus settlementStatus) { + public void updateStatus(@NotNull SettlementStatus settlementStatus) { this.settlementStatus = settlementStatus; }추가로 상태 전이 규칙(예: REQUESTED -> PENDING -> COMPLETED/FAILED)을 도메인 레벨에서 검증하도록 가드 로직을 고려해 보세요.
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java (1)
81-83: 같은 트랜잭션 내 엔티티 재조회 및 예외 타입 통일 제안동일 Tx에서 직전에 조회/수정한 Wallet을 다시 findById로 재조회할 필요가 없습니다. 불필요한 DB 접근을 줄이기 위해 Wallet 엔티티 자체를 파라미터로 받는 형태로 바꾸거나(권장), 최소한 예외는 도메인 예외로 통일하는 편이 좋습니다.
최소 변경(diff):
- Wallet wallet = walletRepository.findById(walletId).orElseThrow(); - Wallet leaderWallet = walletRepository.findById(leaderWalletId).orElseThrow(); + Wallet wallet = walletRepository.findById(walletId) + .orElseThrow(() -> new CustomException(ErrorCode.WALLET_NOT_FOUND)); + Wallet leaderWallet = walletRepository.findById(leaderWalletId) + .orElseThrow(() -> new CustomException(ErrorCode.WALLET_NOT_FOUND));선택 리팩터링: 메서드 시그니처를 (Wallet wallet, Wallet leaderWallet, int amount, UserSettlement userSettlement)로 변경하면 재조회를 제거할 수 있습니다. 원하시면 호출부까지 포함해 안전한 시그니처 변경 패치를 준비하겠습니다.
src/main/java/com/example/onlyone/domain/settlement/service/SettlementService.java (1)
151-162: 위임 시 재조회 최소화 및 책임 경계 명확화 제안현재 잔액 갱신 → walletService.createSuccessfulWalletTransactions(walletId, leaderWalletId, ...) → walletRepository.save(...) 순서입니다. 같은 Tx 내에서 이미 로딩/수정한 엔티티를 WalletService에서 다시 findById로 재조회하고 있어 불필요한 쿼리가 발생할 수 있습니다. 다음을 고려해 주세요.
- WalletService 메서드가 Wallet 엔티티를 직접 받도록 시그니처 변경해 재조회를 제거.
- WalletService는 “트랜잭션/Transfer 기록”만 수행하고, UserSettlement 상태 변경은 SettlementService에서만 수행(현재 중복 변경은 WalletService 쪽 제거 권고 — 별도 코멘트 참조).
예시(요청 시 전체 패치 제공 가능):
- walletService.createSuccessfulWalletTransactions(wallet, leaderWallet, amount, userSettlement)
- WalletService 내부의 findById 제거
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/example/onlyone/domain/payment/controller/PaymentController.java(1 hunks)src/main/java/com/example/onlyone/domain/payment/entity/Payment.java(1 hunks)src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java(1 hunks)src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java(2 hunks)src/main/java/com/example/onlyone/domain/settlement/entity/SettlementStatus.java(1 hunks)src/main/java/com/example/onlyone/domain/settlement/entity/UserSettlement.java(1 hunks)src/main/java/com/example/onlyone/domain/settlement/service/SettlementService.java(6 hunks)src/main/java/com/example/onlyone/domain/wallet/entity/WalletTransactionStatus.java(1 hunks)src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java(3 hunks)src/main/java/com/example/onlyone/global/exception/ErrorCode.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T07:54:25.883Z
Learnt from: ghkddlscks19
PR: GoormOnlyOne/OnlyOne-Back#86
File: src/main/java/com/example/onlyone/domain/club/repository/ClubRepository.java:14-18
Timestamp: 2025-08-06T07:54:25.883Z
Learning: In the OnlyOne-Back project, the ClubRepository search methods use List<Object[]> return type instead of Page<Object[]> even with Pageable parameters. This is intentionally designed for infinite scrolling implementation on the frontend, where only the actual data is needed without pagination metadata.
Applied to files:
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java
🔇 Additional comments (6)
src/main/java/com/example/onlyone/global/exception/ErrorCode.java (1)
71-71: 정산 실패 공통 에러 코드 추가 LGTM정산 실패 공통 코드(SETTLEMENT_PROCESS_FAILED) 추가로 장애 구분/관측성에 도움이 됩니다. 해당 코드가 실제로 로깅/알림 라인에 매핑되는지(예: Sentry, APM 태그)만 확인 부탁드립니다.
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java (1)
118-121: 실패 기록을 별도 트랜잭션으로 격리(REQUIRES_NEW): 적절합니다실패 시점의 지갑 잔액 및 트랜잭션 기록을 외부 롤백과 분리하여 남기는 설계는 타당합니다. 운영 중 장애 추적 및 회계적 감사 용이성이 높아집니다.
src/main/java/com/example/onlyone/domain/settlement/service/SettlementService.java (4)
101-101: 총액 계산식에서 리더 몫 제외: 적절한 변경(userCount - 1) * cost로 총액을 산정하여 리더 본인 부담을 제외하는 방향은 비즈니스 의도에 부합합니다.
122-122: 롤백 범위 확대(@transactional(rollbackFor = Exception.class)): 적절정산 처리 중 체크 예외까지 롤백 대상으로 포함하는 설정은 실패 시 일관성 유지를 돕습니다.
173-180: 실패 처리 플로우 정돈: 로그/기록/상태 변경/예외 변환 순서 적절실패 로그 → 실패 트랜잭션 기록(REQUIRES_NEW) → UserSettlement FAILED 저장 → 도메인 예외 변환의 플로우는 명확하고 운영상 관찰 가능성을 높입니다.
141-145: PESSIMISTIC_WRITE 잠금이 이미 적용되어 있습니다
WalletRepository.findByUser(...)에@Lock(LockModeType.PESSIMISTIC_WRITE)가 선언되어 있고,SettlementService는 클래스 단위에@Transactional이 적용되어 있으므로 해당 메서드 호출 시 row-level 락이 정상 작동하여 동시성 이슈(이중 지출)를 방지합니다.
추가 조치 없이 현재 구현으로 충분하며, 필요 시 엔티티에@Version을 추가해 낙관적 락을 병행 도입할 수 있습니다.
src/main/java/com/example/onlyone/domain/payment/controller/PaymentController.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/settlement/entity/SettlementStatus.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/wallet/entity/WalletTransactionStatus.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/wallet/service/WalletService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/example/onlyone/domain/club/service/ClubService.java (1)
150-164: 주석 처리된 죽은 코드 정리 필요SearchService 경로로 기능이 이관된 상태에서 과거 API 코드가 주석으로 남아 있습니다. 특히 내부에 오타로 보이는
findByUserUs호출이 포함되어 있어 혼선을 유발할 수 있습니다. 코드베이스 일관성과 가독성을 위해 해당 블록은 삭제하시길 권장합니다.적용 예시:
-// /* 가입하고 있는 모임 조회*/ -// public List<ClubNameResponseDto> getClubNames() { -// Long userId = userService.getCurrentUser().getUserId(); -// -// List<UserClub> userClubs = userClubRepository.findByUserUs(userId); -// -// return userClubs.stream() -// .map(UserClub::getClub) -// .filter(Objects::nonNull) -// .map(c -> ClubNameResponseDto.builder() -// .clubId(c.getClubId()) -// .name(c.getName()) -// .build()) -// .toList(); -// }src/main/java/com/example/onlyone/domain/search/controller/SearchController.java (1)
80-84: 메서드 명칭과 응답 빌더 일관성 개선 제안
- 메서드명이
getClubNames이지만ClubResponseDto목록을 반환합니다. 의미를 반영하여getMyClubs등으로 명확화하는 것을 권장합니다.- 본 컨트롤러의 다른 메서드와 동일하게
ResponseEntity.ok(...)를 사용하면 일관성이 좋아집니다.- @GetMapping("/user") - public ResponseEntity<?> getClubNames() { - return ResponseEntity.status(HttpStatus.OK).body(CommonResponse.success(searchService.getMyClubs())); - } + @GetMapping("/user") + public ResponseEntity<?> getMyClubs() { + return ResponseEntity.ok(CommonResponse.success(searchService.getMyClubs())); + }src/main/java/com/example/onlyone/domain/search/service/SearchService.java (1)
186-190: count 캐스팅을 Number 기반으로 변경하여 Dialect 간 호환성과 일관성 확보동일 파일의 다른 변환 로직에서처럼
(Number)...longValue()패턴을 사용하면 JPA 제공자별 숫자 타입 차이(BigInteger/Long 등)에 더 안전합니다. 또한 프로젝트 전반의 일관성도 맞출 수 있습니다.- return rows.stream().map(row -> { - Club club = (Club) row[0]; - Long memberCount = (Long) row[1]; - return ClubResponseDto.from(club, memberCount, true); - }).toList(); + return rows.stream().map(row -> { + Club club = (Club) row[0]; + long memberCount = ((Number) row[1]).longValue(); + return ClubResponseDto.from(club, memberCount, true); + }).toList();src/main/java/com/example/onlyone/domain/club/repository/UserClubRepository.java (1)
26-36: 서브쿼리 기반 count의 성능 비용 고려 (선택) — 연관 조인으로 대체 가능 시 검토현재 쿼리는 클럽별 멤버 수를 서브쿼리로 계산합니다. 데이터 규모가 커질 경우, 연관 조인 + group by가 더 효율적일 수 있습니다. 단, 이는
Club엔티티에userClubs연관관계가 존재한다는 전제입니다.가능한 대안(연관관계 존재 시):
@Query(""" select c, count(uc2) from UserClub uc join uc.club c left join c.userClubs uc2 where uc.user.userId = :userId group by c """) List<Object[]> findMyClubsWithMemberCount(@Param("userId") Long userId);팀 합의상 현재 프로젝트는 목록 무한스크롤을 위해 List<Object[]>를 사용하는 것으로 알고 있으니(사내 컨벤션) 반환 타입은 유지하는 방향이 적절해 보입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/example/onlyone/domain/club/controller/ClubController.java(0 hunks)src/main/java/com/example/onlyone/domain/club/repository/UserClubRepository.java(1 hunks)src/main/java/com/example/onlyone/domain/club/service/ClubService.java(1 hunks)src/main/java/com/example/onlyone/domain/search/controller/SearchController.java(2 hunks)src/main/java/com/example/onlyone/domain/search/service/SearchService.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/example/onlyone/domain/club/controller/ClubController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T07:54:25.883Z
Learnt from: ghkddlscks19
PR: GoormOnlyOne/OnlyOne-Back#86
File: src/main/java/com/example/onlyone/domain/club/repository/ClubRepository.java:14-18
Timestamp: 2025-08-06T07:54:25.883Z
Learning: In the OnlyOne-Back project, the ClubRepository search methods use List<Object[]> return type instead of Page<Object[]> even with Pageable parameters. This is intentionally designed for infinite scrolling implementation on the frontend, where only the actual data is needed without pagination metadata.
Applied to files:
src/main/java/com/example/onlyone/domain/club/repository/UserClubRepository.javasrc/main/java/com/example/onlyone/domain/search/service/SearchService.javasrc/main/java/com/example/onlyone/domain/search/controller/SearchController.java
src/main/java/com/example/onlyone/domain/club/repository/UserClubRepository.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java (1)
97-106: 게이트웨이 응답-요청 교차 검증 누락응답의 orderId가 요청/시스템에 저장된 값과 일치하는지 검증해 위변조/불일치 케이스를 차단하세요.
try { response = tossPaymentClient.confirmPayment(req); } catch (FeignException.BadRequest e) { throw new CustomException(ErrorCode.INVALID_PAYMENT_INFO); } catch (FeignException e) { throw new CustomException(ErrorCode.TOSS_PAYMENT_FAILED); } catch (Exception e) { throw new CustomException(ErrorCode.INTERNAL_SERVER_ERROR); } + if (!req.getOrderId().equals(response.getOrderId())) { + throw new CustomException(ErrorCode.INVALID_PAYMENT_INFO); + }
♻️ Duplicate comments (4)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java (2)
25-25: 스타일: 불필요한 이중 공백 제거가독성/컨벤션 정합성을 위해 공백을 한 칸으로 정리하세요.
- @Column(name = "toss_order_id", nullable = false, unique = true) + @Column(name = "toss_order_id", nullable = false, unique = true)
25-26: toss_order_id에 unique 추가: 실패 기록 ↔ 승인 재시도 충돌 위험(정책/로직 보완 필요)fail(reportFail)에서 해당 orderId로 Payment가 이미 생성되면, confirm에서 paymentKey 기준 조회 미스 시 같은 orderId로 신규 INSERT 시도하다가 유니크 제약에 걸립니다. 정책적으로 "orderId 단건 멱등"을 원한다면:
- confirm/reportFail 모두에서 orderId 기반 조회/멱등 체크를 병행(또는 우선)하고,
- 신규 생성 시에는 반드시 tossPaymentKey와 method를 포함해 저장,
- 경쟁 상태에서는 유니크 예외를 idempotent하게 재조회 처리하세요.
선호안:
- Repository에 findByTossOrderId/existsByTossOrderId 추가(잠금 포함) 후 Service에서 orderId 우선 재사용 로직 도입.
src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java (1)
15-15: 멱등성 보완: orderId 기반 존재/조회 메서드도 추가하세요실패(reportFail) 경로에서 orderId만 먼저 저장된 후, 승인(confirm) 경로가 paymentKey로만 조회하면 유니크(toss_order_id) 충돌이 납니다. 아래 메서드 추가를 권장합니다.
public interface PaymentRepository extends JpaRepository<Payment, Long> { @Lock(LockModeType.PESSIMISTIC_WRITE) Optional<Payment> findByTossPaymentKey(String tossPaymentKey); boolean existsByTossPaymentKey(String paymentKey); + + @Lock(LockModeType.PESSIMISTIC_WRITE) + Optional<Payment> findByTossOrderId(String tossOrderId); + boolean existsByTossOrderId(String tossOrderId); }src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java (1)
129-166: reportFail: 멱등성/유니크 충돌/필수 필드 검증 보완 필요현재 로직은
- (1) paymentKey 미존재 시 tossPaymentKey/method 없이 INSERT 시도 → not-null 제약 위반,
- (2) orderId 유니크와 충돌 가능,
- (3) 요청 필드 null 검증 부재.
아래 보완을 권장합니다.
@Transactional(propagation = Propagation.REQUIRES_NEW) public void reportFail(ConfirmTossPayRequest req) { - // 멱등성, 동시성 보호: paymentKey로 행 잠금 - Payment payment = paymentRepository.findByTossPaymentKey(req.getPaymentKey()) - .orElseGet(() -> { - Payment p = Payment.builder() - .tossOrderId(req.getOrderId()) - .totalAmount(req.getAmount()) - .status(Status.IN_PROGRESS) - .build(); - return paymentRepository.save(p); - }); + // 입력 유효성 + if (req.getPaymentKey() == null || req.getOrderId() == null) { + throw new CustomException(ErrorCode.INVALID_PAYMENT_INFO); + } + // 멱등성/동시성: paymentKey 우선, 없으면 orderId로 재사용 + Payment payment = paymentRepository.findByTossPaymentKey(req.getPaymentKey()) + .orElseGet(() -> + paymentRepository.findByTossOrderId(req.getOrderId()) + .orElseGet(() -> { + try { + return paymentRepository.save( + Payment.builder() + .tossPaymentKey(req.getPaymentKey()) + .tossOrderId(req.getOrderId()) + .method(Method.from(req.getMethod())) + .totalAmount(req.getAmount()) + .status(Status.IN_PROGRESS) + .build()); + } catch (org.springframework.dao.DataIntegrityViolationException e) { + return paymentRepository.findByTossOrderId(req.getOrderId()) + .orElseThrow(() -> new CustomException(ErrorCode.INVALID_PAYMENT_INFO)); + } + }) + ); // 이미 완료된 결제면 기록하지 않음 if (payment.getStatus() == Status.DONE) { return; } - if (req.getPaymentKey() != null && - payment.getTossPaymentKey() != null && - payment.getTossPaymentKey().equals(req.getPaymentKey())) { - return; - } + // 동일 키 재시도 멱등 처리 + if (req.getPaymentKey().equals(payment.getTossPaymentKey())) return;또한 실패 기록 중복을 억제하려면 orderId 존재 검사도 병행:
if (paymentRepository.existsByTossOrderId(req.getOrderId())) return;WalletTransaction 중복 정책(실패 로그 누적 vs 단건 멱등)도 명확화가 필요합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java(2 hunks)src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java(1 hunks)src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java(4 hunks)
🔇 Additional comments (3)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java (1)
42-44: updateStatus(Status) 추가: 적절합니다엔티티 상태 전이를 캡슐화하여 서비스 레이어 가독성이 좋아집니다.
src/main/java/com/example/onlyone/domain/payment/repository/PaymentRepository.java (1)
12-13: paymentKey 기준 비관적 잠금: 적절합니다동일 paymentKey 동시 처리 경합 시 안전합니다. 신규 행 생성 경합도 함께 고려해 예외 기반 재조회 패턴을 서비스에서 병행하세요.
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java (1)
111-119: 금액 변환 시 INT 오버플로우 위험 점검 요망
response.getTotalAmount()가 long 또는 BigDecimal 형태로 반환될 경우, Math.toIntExact 사용 시 예외 발생 또는 값 손실이 발생할 수 있습니다. 또한WalletTransaction.amount는 int 타입이므로 변환 전 타입과 범위를 반드시 검토해야 합니다.– ConfirmTossPayResponse.totalAmount getter 반환 타입 확인
– Wallet.balance 필드 및 getBalance() 반환 타입 확인
– WalletTransaction.amount 필드 타입 확인범위 검사 또는 long 유지 방안을 도입하세요.
예시 리팩토링:- int amount = Math.toIntExact(response.getTotalAmount()); + long amountL = response.getTotalAmount(); + if (amountL > Integer.MAX_VALUE) { + throw new CustomException(ErrorCode.INVALID_PAYMENT_INFO); + } + int amount = (int) amountL;
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
Show resolved
Hide resolved
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java (2)
84-93: Payment 생성 시 필수 필드 누락으로 인한 제약 위반 위험Payment 생성 시
tossPaymentKey와method필드가 누락되어 DB 제약 위반이 발생할 수 있습니다. 이는 이전 리뷰에서도 지적된 문제입니다.Payment p = Payment.builder() + .tossPaymentKey(req.getPaymentKey()) .tossOrderId(req.getOrderId()) - .tossOrderId(req.getOrderId()) + .method(Method.UNKNOWN) // 기본값 또는 req에서 가져오기 .status(Status.IN_PROGRESS) .totalAmount(req.getAmount()) .build();
133-141: reportFail에서도 동일한 필수 필드 누락 문제confirm 메서드와 동일하게
tossPaymentKey와method필드가 누락되어 있습니다.Payment p = Payment.builder() + .tossPaymentKey(req.getPaymentKey()) .tossOrderId(req.getOrderId()) .totalAmount(req.getAmount()) + .method(Method.UNKNOWN) .status(Status.IN_PROGRESS) .build();
🧹 Nitpick comments (1)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java (1)
3-3: 사용되지 않는 import 제거 필요
ConfirmTossPayResponse가 import되었지만 이 엔티티 클래스에서 사용되지 않습니다.-import com.example.onlyone.domain.payment.dto.response.ConfirmTossPayResponse;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java(3 hunks)src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T09:51:57.965Z
Learnt from: gkdudans
PR: GoormOnlyOne/OnlyOne-Back#110
File: src/main/java/com/example/onlyone/domain/wallet/entity/WalletTransactionStatus.java:5-5
Timestamp: 2025-08-11T09:51:57.965Z
Learning: In the OnlyOne project's wallet transaction system (WalletTransactionStatus enum in src/main/java/com/example/onlyone/domain/wallet/entity/WalletTransactionStatus.java), the PENDING status is used as a locking mechanism for concurrency control to prevent duplicate transaction processing, not for retry or timeout handling.
Applied to files:
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java
🔇 Additional comments (5)
src/main/java/com/example/onlyone/domain/payment/entity/Payment.java (3)
26-26: toss_order_id unique 제약 조건 추가 승인멱등성 보장을 위한 unique 제약 조건 추가가 적절합니다. 이전 리뷰에서 논의된 대로 PaymentService에서 적절한 멱등성 처리가 구현되어 있는지 확인이 필요합니다.
33-33: method 필드 nullable 제약 제거 적절PaymentService에서 payment 생성 시점에 method 정보가 없을 수 있으므로 nullable 제약 제거가 합리적입니다.
43-50: 비즈니스 로직을 담는 메서드들 적절히 구현됨
updateStatus와updateOnConfirm메서드가 잘 구현되었습니다. 특히updateOnConfirm은 결제 승인 시 상태와 결제수단을 동시에 업데이트하는 편의 메서드로 유용합니다.src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java (2)
125-125: updateOnConfirm 호출이 올바르게 수정됨이전 리뷰에서 지적된 대로
updateOnConfirm메서드가 적절히 호출되고 있습니다. Payment 엔티티의 새로운 메서드와 잘 연동됩니다.
130-131: REQUIRES_NEW 트랜잭션 전파 적절실패 보고를 별도 트랜잭션으로 처리하는 것이 적절합니다. 메인 트랜잭션이 실패해도 실패 로그가 독립적으로 기록될 수 있습니다.
| .orElseGet(() -> { | ||
| Payment p = Payment.builder() | ||
| .tossOrderId(req.getOrderId()) | ||
| .tossOrderId(req.getOrderId()) |
There was a problem hiding this comment.
중복된 tossOrderId 설정
tossOrderId가 두 번 설정되어 있습니다.
Payment p = Payment.builder()
.tossOrderId(req.getOrderId())
- .tossOrderId(req.getOrderId())
.status(Status.IN_PROGRESS)📝 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.
| .tossOrderId(req.getOrderId()) | |
| Payment payment = paymentRepository.findByTossPaymentKey(req.getPaymentKey()) | |
| .orElseGet(() -> { | |
| Payment p = Payment.builder() | |
| .tossOrderId(req.getOrderId()) | |
| .status(Status.IN_PROGRESS) | |
| .totalAmount(req.getAmount()) | |
| .build(); | |
| return paymentRepository.save(p); | |
| }); |
🤖 Prompt for AI Agents
In src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
around line 88, the builder/setter call .tossOrderId(req.getOrderId()) is
applied twice; remove the duplicate invocation (or if the second was meant to
set a different field, replace it with the correct setter) so tossOrderId is set
only once and the builder state is unambiguous.
| if (req.getPaymentKey() != null && | ||
| payment.getTossPaymentKey() != null && | ||
| payment.getTossPaymentKey().equals(req.getPaymentKey())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이상한 멱등성 체크 로직
이 멱등성 체크 로직이 부자연스럽습니다. 이미 findByTossPaymentKey로 조회한 payment이므로 paymentKey가 일치하는 것은 당연합니다. 불필요한 코드입니다.
- if (req.getPaymentKey() != null &&
- payment.getTossPaymentKey() != null &&
- payment.getTossPaymentKey().equals(req.getPaymentKey())) {
- return;
- }📝 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.
| if (req.getPaymentKey() != null && | |
| payment.getTossPaymentKey() != null && | |
| payment.getTossPaymentKey().equals(req.getPaymentKey())) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/onlyone/domain/payment/service/PaymentService.java
around lines 146-150, the if-block that checks req.getPaymentKey() against
payment.getTossPaymentKey() is redundant because payment was already retrieved
by that key; remove this entire conditional and its return to simplify the flow
(ensure any intended idempotency is covered by the earlier lookup path or by
more appropriate checks elsewhere).
2 participants
#️⃣ Issue Number
📝 요약(Summary)
정산 / 충전 관련 실패 로직 추가
🛠️ PR 유형
어떤 변경 사항이 있나요?
📸스크린샷 (선택)
💬 공유사항 to 리뷰어
✅ PR Checklist
PR이 다음 요구 사항을 충족하는지 확인하세요.
Summary by CodeRabbit
New Features
Improvements
Behavior Changes