Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAccount 엔티티의 잔액/버전 필드 재구성과 LedgerService 도입으로 동전 이체가 분산 잠금 기반 원장(ledger) 방식으로 변경되었고, AiImagePermissionService는 동전 소비(consume)에서 이체(transfer) 호출로 대체되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Buyer as 구매자
participant AiService as AiImagePermissionService
participant Ledger as LedgerService
participant LockMgr as LockManager
participant AccRepo as AccountRepository
participant TxRepo as CoinTransactionEntityRepository
Buyer->>AiService: buyAiImagePermissionByCoin(postId)
AiService->>Ledger: transfer(buyerId, sellerId, price)
Ledger->>LockMgr: multipleAccountLock([buyerId,sellerId], block)
rect rgb(230,245,255)
Note over LockMgr: 잠금 획득 및 원장 업데이트
LockMgr->>AccRepo: load(buyerId), load(sellerId)
AccRepo-->>LockMgr: accountBuyer, accountSeller
LockMgr->>LockMgr: accountBuyer.updateBalance(newBuyerBalance)
LockMgr->>LockMgr: accountSeller.updateBalance(newSellerBalance)
LockMgr->>TxRepo: save(transactionEntity: debit), save(transactionEntity: credit)
LockMgr->>AccRepo: save(accountBuyer), save(accountSeller)
end
LockMgr-->>Ledger: 완료
Ledger-->>AiService: transfer 완료
AiService->>AiService: upsertAndIncrease(permission)
AiService->>AiService: sendNotification()
AiService-->>Buyer: 권한 구매 완료
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (7)
src/test/java/hanium/modic/backend/domain/transaction/AccountServiceIntegrationTest.java (1)
35-85: 동시성 시나리오에서 송신자(B,C)의 최종 잔액도 함께 검증하는 것이 좋습니다현재 테스트는 B,C가 동시에 A에게 500씩 전송했을 때 A의
postedBalance가 1000이 되는지만 검증합니다. 복식부기 관점에서:
- B,C 각각의 최종 잔액이 0이 되었는지
- (필요하다면) 관련
CoinTransaction/엔트리 개수까지를 추가로 검증하면, LedgerService와 Account 엔티티 변경이 더 잘 보호됩니다. 또한 테스트 종료 후
executor.shutdown()정도로 스레드를 정리해 두면 장기적으로 테스트 안정성에도 도움이 됩니다.src/test/java/hanium/modic/backend/web/ai/controller/AiImagePermissionControllerIntegrationTest.java (2)
66-109: 성공 케이스에서 판매자 계좌의 증가도 함께 검증하는 것을 추천합니다
buyAiImagePermissionWithCoin_Success에서
- buyer 계좌를
updateBalance(10000L)로 초기화하고,- seller 유저/계좌/포스트를 분리해 생성한 뒤,
- buyer의
postedBalance == 5000L만 검증하고 있습니다. 복식부기/코인 양도 관점에서는 seller 계좌의
postedBalance가 5000L로 증가했는지도 같이 검증하면, LedgerService까지 포함한 전체 플로우가 더 견고하게 커버됩니다.예시:
- Account updatedAccount = accountRepository.findById(user.getId()).orElse(null); - assertThat(updatedUser).isNotNull(); - assertThat(updatedAccount.getPostedBalance()).isEqualTo(5000L); // 10000 - 5000 + Account updatedBuyerAccount = accountRepository.findById(user.getId()).orElse(null); + Account updatedSellerAccount = accountRepository.findById(user2.getId()).orElse(null); + + assertThat(updatedUser).isNotNull(); + assertThat(updatedBuyerAccount.getPostedBalance()).isEqualTo(5000L); // 10000 - 5000 + assertThat(updatedSellerAccount).isNotNull(); + assertThat(updatedSellerAccount.getPostedBalance()).isEqualTo(5000L); // 0 + 5000
150-178: 코인 부족 케이스에서 권한 upsert 롤백 여부를assert로 명시해 주세요
buyAiImagePermissionWithCoin_InsufficientCoin테스트에서:boolean permissionExists = aiChatRoomRepository.existsByUserIdAndPostId(user.getId(), post.getId()); // upsert가 먼저 실행되므로 권한은 생성되지만 코인 차감에서 실패해야 함 // 실제로는 트랜잭션 롤백이 되어야 하지만, 현재 구조상 체크까지 계산/주석은 있지만,
permissionExists에 대한assert가 없어 실제 기대 동작이 테스트에 드러나지 않습니다.
- 이번 PR에서 코인 양도/원장 처리가 트랜잭션 안으로 들어갔다면, 권한 upsert도 함께 롤백되어
permissionExists가false가 되는 것이 이상적일 수 있고,- 기존 구조를 유지한다면 반대로
true를 기대할 수도 있습니다.어느 쪽이든 현재 의도를
assertThat(permissionExists).isFalse()혹은isTrue()로 명확히 드러내 두면, 이후 트랜잭션 구조 변경 시 회귀를 잘 잡을 수 있습니다.src/main/java/hanium/modic/backend/domain/transaction/service/LedgerService.java (1)
51-75:ledgerVersion캡처 시점과 락/예외 처리 방식 의도 확인 제안몇 가지 설계 포인트를 한 번 더 확인해 보시는 것이 좋겠습니다.
ledgerVersion캡처 시점// 3) 버전 캡처, 반드시 Account 업데이트 이후에 호출되어야 함 long fromVersionBefore = fromAccount.getLedgerVersion(); long toVersionBefore = toAccount.getLedgerVersion();주석은 “업데이트 이후” 캡처를 의도하지만 변수명은
*Before라 약간 혼동의 여지가 있습니다. 또한updateBalance가ledgerVersion을 증가시키는지, 아니면 별도 메서드가 있는지 이 코드만으로는 보이지 않습니다.
→ 엔트리의version필드가 “업데이트 전”인지 “업데이트 후”인지를 명확히 정의하고, 그에 맞춰 변수명/주석을 정리해 두면 이후 리팩토링 때 버그를 막는 데 도움이 됩니다.
multipleAccountLock의 예외 전달 방식
- 현재는
LockException만 잡아COIN_TRANSFER_FAIL_EXCEPTION으로 래핑하고 있고,- 콜백 내부에서 발생하는
AppException은 그대로 전파된다는 가정을 두고 있습니다.실제 구현에서 콜백 예외를
LockException으로 감싸지 않는지, 획득 실패와 비즈니스 예외가 명확히 구분되는지 한 번 더 확인해 두면 장애 원인 파악이 쉬워집니다.이 부분들은 기능 버그라기보다는 향후 유지보수성을 높이기 위한 설계 확인/정리 포인트로 보입니다.
src/main/java/hanium/modic/backend/domain/transaction/service/AccountService.java (1)
23-64: LedgerService 위임 구조는 명확하지만, 알림을 동일 트랜잭션에 포함하는 의도는 한 번 더 정리해 두면 좋겠습니다이번 변경으로:
- 계좌/원장 관련 변경은 모두
LedgerService.transfer로 위임되고,AccountService.transferCoin은
- 자기 자신 양도 방지,
- 수신자 존재 확인,
- LedgerService 호출,
- 알림 발송
의 역할만 가지는 구조가 되어 책임 분리가 깔끔합니다.
다만
@Transactional이transferCoin전체에 걸려 있어, 알림 생성 실패 시 코인 양도/원장 기록까지 롤백되는 구조입니다. “양도는 반드시 성공해야 하고, 알림은 부가 기능” 이라는 요구사항이라면:
- LedgerService 호출 부분만을 포함하는 별도 트랜잭션으로 두고,
- 알림은 트랜잭션 밖(또는 별도 비동기 플로우)에서 처리하는 방향
을 고려해 볼 여지가 있습니다. 반대로 “알림까지 포함해서 모두 하나의 트랜잭션으로 간주”하는 것이 의도라면, 이 사실을 메서드 주석 등으로 명시해 두면 이후 유지보수 시 혼동을 줄일 수 있습니다.
src/main/java/hanium/modic/backend/domain/transaction/entity/Account.java (2)
31-36:ledgerVersion/postedBalance기본값 및 타입 선택 점검 제안두 필드를
nullable = false+Long래퍼 + 필드 초기값0L로 두신 설계는 기능상 문제는 없어 보입니다. 다만 DB 컬럼이 이미 존재한다면, 기존 레코드에서NULL값이 남아 있지 않은지 한 번 더 확인해 두는 게 안전합니다. 또한 절대null이 될 수 없는 값이라면 필드 타입을long(primitive)으로 두는 것도 NPE 리스크를 줄이는 선택지입니다.
47-55:updateBalance파라미터 null 처리 및 검증 강화 권장
updateBalance(Long newBalance)에서newBalance < 0비교는 호출 측에서null을 넘기면 NPE가 날 수 있습니다. 비즈니스 상null이 유효하지 않다면:
- 시그니처를
updateBalance(long newBalance)로 바꾸거나,- 메서드 초기에
Objects.requireNonNull(newBalance, "...")등으로 명시적으로 검증하는 것을 권장합니다.또한 현재는 음수만 막고 있어 오버플로 등은 상위 레이어에서 이미 방어한다고 가정하는 설계로 보이는데, 만약 장부에서 직접 합산하여
newBalance를 계산한다면 이 메서드가 “최종 방어선”인지 역할을 명확히 해 두면 유지보수에 도움이 됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/hanium/modic/backend/domain/ai/aiChat/service/AiImagePermissionService.java(1 hunks)src/main/java/hanium/modic/backend/domain/profile/service/ProfileService.java(2 hunks)src/main/java/hanium/modic/backend/domain/transaction/entity/Account.java(2 hunks)src/main/java/hanium/modic/backend/domain/transaction/entity/CoinTransactionEntity.java(1 hunks)src/main/java/hanium/modic/backend/domain/transaction/repository/AccountRepository.java(1 hunks)src/main/java/hanium/modic/backend/domain/transaction/repository/CoinTransactionEntityRepository.java(1 hunks)src/main/java/hanium/modic/backend/domain/transaction/repository/CoinTransactionRepository.java(1 hunks)src/main/java/hanium/modic/backend/domain/transaction/service/AccountService.java(3 hunks)src/main/java/hanium/modic/backend/domain/transaction/service/LedgerService.java(1 hunks)src/main/java/hanium/modic/backend/infra/redis/distributedLock/LockManager.java(1 hunks)src/main/java/hanium/modic/backend/web/transaction/controller/AccountController.java(1 hunks)src/test/java/hanium/modic/backend/domain/ai/service/AiImagePermissionServiceTest.java(4 hunks)src/test/java/hanium/modic/backend/domain/transaction/AccountServiceIntegrationTest.java(3 hunks)src/test/java/hanium/modic/backend/web/ai/controller/AiImagePermissionControllerIntegrationTest.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/test/java/hanium/modic/backend/domain/transaction/AccountServiceIntegrationTest.java (1)
src/test/java/hanium/modic/backend/domain/user/factory/UserFactory.java (1)
UserFactory(9-29)
src/main/java/hanium/modic/backend/domain/transaction/entity/CoinTransactionEntity.java (1)
src/main/java/hanium/modic/backend/domain/transaction/entity/CoinTransaction.java (1)
Table(24-50)
src/main/java/hanium/modic/backend/domain/transaction/service/LedgerService.java (3)
src/main/java/hanium/modic/backend/domain/ai/aiChat/service/AiImagePermissionService.java (1)
Service(25-139)src/main/java/hanium/modic/backend/domain/transaction/service/AccountService.java (1)
Service(19-65)src/main/java/hanium/modic/backend/common/error/exception/LockException.java (1)
LockException(3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
src/main/java/hanium/modic/backend/domain/transaction/entity/CoinTransactionEntity.java (1)
26-26: 테이블 이름 오타 수정 완료
coin_tranaction_entities에서coin_transaction_entities로 수정되었습니다. 이 수정은 데이터베이스 테이블명과의 일치를 보장하며, 런타임 오류를 방지합니다.src/main/java/hanium/modic/backend/domain/transaction/repository/AccountRepository.java (1)
10-11: LGTM!
findByUserId메서드는 Spring Data JPA 규칙에 따라 올바르게 정의되었으며,Optional<Account>를 반환하여 null 안전성을 제공합니다. 이는 새로운 ledger 기반 전송 흐름에서 userId로 계좌를 조회하는 데 필요한 메서드입니다.src/main/java/hanium/modic/backend/domain/profile/service/ProfileService.java (1)
52-52: LGTM!
Account엔티티의 필드명 변경(coin→postedBalance)에 맞춰 올바르게 수정되었습니다. 이는 복식부기 시스템에서 게시된(posted) 잔액을 나타내는 의미론적으로 더 명확한 이름입니다.src/main/java/hanium/modic/backend/domain/ai/aiChat/service/AiImagePermissionService.java (1)
50-51: LGTM! 코인 전송 로직이 올바르게 변경됨기존의
consumeCoin에서transferCoin으로 변경되어, 이제 구매자(userId)로부터 원작자(post.getUserId())에게 코인이 전송됩니다. 이는 경제적 흐름을 정확히 반영하며, 주석("코인을 원작자에게 전송")과도 일치합니다.
transferCoin이 별도의 트랜잭션으로 동작하며, 실패 시 예외가 전파되어 외부 트랜잭션을 롤백시키는 것으로 보입니다. PR 목표에 명시된 대로 분산 락 내에서 원자적으로 처리되는 것으로 이해됩니다.src/main/java/hanium/modic/backend/domain/transaction/repository/CoinTransactionEntityRepository.java (1)
1-8: LGTM!표준 Spring Data JPA 리포지토리가 올바르게 정의되었습니다.
CoinTransactionEntity에 대한 기본 CRUD 작업을 제공하며, ledger 시스템에서 복식부기의 개별 항목(debit/credit entries)을 저장하는 데 사용됩니다.src/main/java/hanium/modic/backend/domain/transaction/repository/CoinTransactionRepository.java (1)
1-8: LGTM!표준 Spring Data JPA 리포지토리가 올바르게 정의되었습니다.
CoinTransaction에 대한 기본 CRUD 작업을 제공하며, ledger 시스템에서 거래 전체를 나타내는 레코드를 저장하는 데 사용됩니다.src/main/java/hanium/modic/backend/web/transaction/controller/AccountController.java (1)
47-53: LGTM!
USER_NOT_FOUND_EXCEPTION이 에러 매핑에 추가되었습니다. 이는transferCoin이 이제 수신자 사용자를 조회해야 하므로, 수신자가 존재하지 않을 경우 발생할 수 있는 예외를 적절히 문서화한 것입니다.src/main/java/hanium/modic/backend/infra/redis/distributedLock/LockManager.java (1)
36-40: 원본 리뷰 댓글은 부정확합니다. 데드락 위험은 이미 내부 구현에서 방지되고 있습니다.
DistributionLockExecutor.withMultiLock()메서드(line 44)에서 이미keys.stream().sorted().toList()로 키를 정렬하고 있습니다. 따라서multipleAccountLock()에서 입력값을 정렬하지 않아도 내부적으로 정렬이 수행되어 데드락이 방지됩니다. 제안된 수정사항(.sorted()추가)은 불필요합니다.Likely an incorrect or invalid review comment.
src/test/java/hanium/modic/backend/domain/ai/service/AiImagePermissionServiceTest.java (1)
79-124:transferCoin호출 시그니처 검증이 잘 반영됨세 테스트 모두에서
AccountService.consumeCoin→transferCoin(fromUserId, toUserId, amount)변경을 정확히 따라가고 있고,
- 성공 케이스:
(buyerId, post.getUserId(), post.getNonCommercialPrice())로 정확히 검증- 포스트 없음 케이스:
transferCoin이 전혀 호출되지 않음을 검증- 코인 부족 케이스:
transferCoin에서COIN_NOT_ENOUGH_EXCEPTION을 던지도록 mock 후, 서비스가 해당 에러 코드를 그대로 노출하는지 검증까지 흐름이 잘 잡혀 있습니다. 단위 테스트 수준에서는 충분히 안정적으로 API 변경을 커버하고 있다고 보입니다.
src/main/java/hanium/modic/backend/domain/transaction/service/LedgerService.java (1)
41-49: 리뷰 코멘트의 핵심 주장이 코드 현실과 맞지 않습니다.검색 결과에 따르면,
Account.updateBalance()메서드(라인 48-51)에 이미 다음과 같은 검증이 있습니다:public void updateBalance(Long newBalance) { if (newBalance < 0) { throw new AppException(COIN_NOT_ENOUGH_EXCEPTION); } // ... }따라서
LedgerService.transfer()의 라인 47-48에서updateBalance()호출 시, 자동으로 음수 잔액 검증이 작동합니다. 리뷰에서 "마이너스 잔액이 그대로 저장될 수 있다"고 지적한 것은 부정확합니다.현재 코드 구조는 계층별 책임 분리 패턴으로, Account 엔티티 계층에서 최종 검증을 담당하고 있습니다. 따라서 제안된 추가 검증은 중복될 수 있습니다.
Likely an incorrect or invalid review comment.
src/main/java/hanium/modic/backend/infra/redis/distributedLock/LockManager.java
Outdated
Show resolved
Hide resolved
1 participant
개요
작업사항
기존 Redis 분산 락 로직에 장부 처리 로직 적용하기
현재 Redis 분산락은 위와 같이 내부적으로 REQUIRED_NEW 트랜잭션으로 새로운 트랜잭션에서 처리하게 끔 해두었다.(트랜잭션 생명주기가 레디스 분산 락 생명주기보다 짧게 가져가기 위하여)
여기서 Transaction을 분산 락 안에서 처리해야할지 밖에서 처리해도 되는지 고민이 되었다.
밖에서 처리하는 경우
이렇게 세 번의 커밋과정을 거쳐야 한다.
여기서 만약 2번 과정까지 성공했는데, 3번에서 실패하면 실제 돈은 전송됐는데, 장부에는 안남는 문제가 생긴다,
그러므로 분산 락 안에서 장부 처리가 이뤄져야 한다.
레디스 분산락에서 고려해야할 케이스
PG사 API가 아니라 내부적으로 처리되는 로직이므로, 별도의 PENDING을 고려할 필요 없었다.
따라서 이 경우 아래와 같이 동작하게 했다.
즉 코인 송금과 Ledger 처리가 Atomicity하게 동작하여야 했다.
따라서 기존 로직을 수정하여
분산 락 안에서 거래, Transaction 저장, Entity 저장이 모두 이뤄지게 했다.
그리고 내부적으로, Transaction과 한 쌍의 Entity(debit, credit)을 모두 저장하여 복식부기를 따르게 했다.
또한 알람 같은 경우 락 내에서 이뤄질 필요가 없었기에
외부에서 처리되게 했다.
고려사항
Summary by CodeRabbit
새로운 기능
버그 수정
Chores
✏️ Tip: You can customize this high-level summary in your review settings.