feat: Introducing payment domain with orchestration-saga for synchronous payment/refund flow#41
Conversation
📝 WalkthroughWalkthroughAdds a payment orchestration flow with compensation logging and retrying, module APIs for point and reservation domains, JPA persistence and repository implementations, controller endpoints and DTOs, plus unit and integration tests; enables Spring Retry at application and test levels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Orchestrator as PaymentOrchestrator
participant PaymentSvc as PaymentService
participant PointApi as PointModuleApi
participant ReservationApi as ReservationModuleApi
participant CompLogSvc as CompensationTxLogService
Client->>Orchestrator: processPayment(userId, price, reservationId)
Orchestrator->>PaymentSvc: initiate(userId, price, reservationId)
PaymentSvc-->>Orchestrator: Payment(PENDING)
Orchestrator->>PointApi: decreaseUserPointBalance(userId, price)
alt point deduction fails
PointApi-->>Orchestrator: PointOperationResult(failed)
Orchestrator->>PaymentSvc: fail(payment)
PaymentSvc-->>Orchestrator: Payment(FAILED)
Orchestrator-->>Client: PaymentProcessResponse(FAILED)
else point deduction succeeds
PointApi-->>Orchestrator: PointOperationResult(success)
Orchestrator->>ReservationApi: confirmReservation(reservationId)
alt reservation confirmation fails
ReservationApi-->>Orchestrator: ReservationOperationResult(failed)
Orchestrator->>PointApi: increaseUserPointBalance(userId, price)
alt compensation succeeds
PointApi-->>Orchestrator: PointOperationResult(success)
else compensation fails
PointApi-->>Orchestrator: PointOperationResult(failed)
Orchestrator->>CompLogSvc: log(userId,reservationId,paymentId,price)
end
Orchestrator->>PaymentSvc: fail(payment)
PaymentSvc-->>Orchestrator: Payment(FAILED)
Orchestrator-->>Client: PaymentProcessResponse(FAILED)
else reservation confirmation succeeds
ReservationApi-->>Orchestrator: ReservationOperationResult(success)
Orchestrator->>PaymentSvc: complete(payment)
PaymentSvc-->>Orchestrator: Payment(COMPLETED)
Orchestrator-->>Client: PaymentProcessResponse(COMPLETED)
end
end
sequenceDiagram
participant Scheduler as CompensationTxRetryScheduler
participant CompLogSvc as CompensationTxLogService
participant PointApi as PointModuleApi
Scheduler->>Scheduler: wait (fixed delay 1m)
Scheduler->>CompLogSvc: getAllRetriables()
CompLogSvc-->>Scheduler: List<CompensationTxLog>
loop each pending log
alt price > 0
Scheduler->>PointApi: increaseUserPointBalance(userId, price)
else price < 0
Scheduler->>PointApi: decreaseUserPointBalance(userId, abs(price))
end
alt operation succeeds
PointApi-->>Scheduler: PointOperationResult(success)
Scheduler->>CompLogSvc: markAsCompleted(log)
else operation fails
PointApi-->>Scheduler: PointOperationResult(failed)
Scheduler->>CompLogSvc: markAsFailed(log)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
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/slam/concertreservation/domain/point/service/PointService.java (1)
54-57:⚠️ Potential issue | 🟡 MinorStray semicolon on line 57.
There's an extra semicolon after the
orElseThrow(...)statement closing parenthesis.Proposed fix
UserPointBalance userPointBalance = userPointBalanceRepository.getBalanceByUserId(userId) .orElseThrow(() -> new UnavailableRequestException(ErrorCode.USER_NOT_FOUND, "해당 회원이 존재하지 않으므로 잔액 조회가 불가합니다.")); - ;
🤖 Fix all issues with AI agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/CompensationTxRetryScheduler.java`:
- Around line 27-31: In CompensationTxRetryScheduler, guard the price==0 case so
it doesn't fall through to decreaseUserPointBalance with 0: update the method
that currently branches on log.getPrice() (using log.getPrice(),
pointModuleApi.increaseUserPointBalance and
pointModuleApi.decreaseUserPointBalance) to explicitly check for price == 0
first, log a warning or error mentioning the unexpected zero-price compensation
(include identifying info like log.getUserId()/log id), and skip calling the
pointModuleApi in that case; keep the existing increase/decrease branches for
positive/negative prices unchanged.
- Around line 19-38: retryFailedCompensations() only reprocesses PENDING because
getAllRetriables() excludes FAILED and markAsFailed() flips status to FAILED, so
logs get only one retry; change the flow so retries continue until a max: either
(A) modify compensationTxLogService.getAllRetriables() to return entries with
status PENDING or FAILED whose retryCount < maxRetries (configurable) and leave
markAsFailed() to only increment retryCount and optionally set status=FAILED
only when retryCount >= maxRetries, or (B) keep getAllRetriables() returning
only PENDING but change markAsFailed(CompensationTxLog) to increment and persist
a retryCount and keep status=PENDING until retryCount exceeds maxRetries then
set FAILED; update CompensationTxLog to track retryCount and ensure retry cap is
enforced before calling markAsCompleted()/final failure.
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/PaymentOrchestrator.java`:
- Around line 52-76: Before creating a PENDING refund in processRefund, fetch
and validate the current reservation and payment state: call
reservationModuleApi.getReservation(reservationId) (or equivalent) and verify
it's in a refundable state (e.g., CONFIRMED) and fetch the existing payment via
paymentService.findByReservationId(reservationId) to ensure a completed charge
exists; if validation fails, return a failure response without calling
paymentService.initiate, pointModuleApi.increaseUserPointBalance,
reservationModuleApi.cancelReservation, or paymentService.refund. Keep existing
compensation logic (compensatePointIncrease and paymentService.fail) for
downstream failures but only after the initial pre-checks pass.
- Around line 48-49: Wrap the final persistence calls
paymentService.complete(initiatedPayment) and
paymentService.refund(initiatedPayment) in a try-catch inside
PaymentOrchestrator so a thrown exception doesn't leave the system with deducted
points and a PENDING payment; on catch, perform a compensating action such as
retrying the persistence (with a limited retry loop/backoff) or
creating/flagging a reconciliation/compensation record (e.g., via a new
paymentRepository.createCompensationEntry or
paymentService.markForReconciliation) and log the full error and context, then
return an appropriate failure response instead of letting the exception escape;
ensure PaymentProcessResponse.from is only called after the persistence
succeeds.
- Line 6: PaymentOrchestrator currently imports and returns the presentation DTO
PaymentProcessResponse, leaking presentation concerns into the
domain/application layer; change the orchestrator to return a domain-level
Payment (or other domain result object) instead of PaymentProcessResponse,
remove the import of
com.slam.concertreservation.interfaces.dto.PaymentProcessResponse from
PaymentOrchestrator, update the orchestrator method signature(s) (e.g.,
processPayment/orchestratePayment) and any internal returns to produce the
domain Payment, and move the mapping to PaymentProcessResponse into the
controller layer so controllers call PaymentOrchestrator to get the domain
Payment and translate it to the DTO for the API response.
In `@src/main/java/com/slam/concertreservation/domain/payment/model/Payment.java`:
- Around line 96-98: The withStatus method currently copies the existing
paymentId and createdAt which violates the append-only design; change
Payment.withStatus to create a new TSID paymentId and set createdAt to now (or
otherwise generate a new creation timestamp) so calling withStatus produces a
new entity/row instead of triggering a JPA merge — update the Payment.withStatus
implementation to return new Payment(newPaymentId, this.userId,
this.reservationId, this.price, paymentStatus, newCreatedAt) and keep the
existing Javadoc about append-only behavior.
In
`@src/main/java/com/slam/concertreservation/domain/payment/service/CompensationTxLogService.java`:
- Around line 13-15: Add transactional boundaries to state-mutating methods in
CompensationTxLogService: annotate the methods log(), markAsFailed(), and
markAsCompleted() with `@Transactional` so their updates around
compensationTxLogRepository.save(...) execute atomically; ensure imports/use of
org.springframework.transaction.annotation.Transactional and keep the
class-level annotations (Service, RequiredArgsConstructor) unchanged.
- Around line 34-36: getAllRetriables() currently returns all CompensationTxLog
entries with CompensationTxStatus.PENDING, which allows logs with retryCount >=
MAX_RETRY_COUNT to be retried indefinitely; update getAllRetriables() to only
return logs whose retryCount is less than MAX_RETRY_COUNT (e.g., call a
repository method like
findAllByStatusAndRetryCountLessThan(CompensationTxStatus.PENDING,
MAX_RETRY_COUNT) or filter the result list by log.getRetryCount() <
MAX_RETRY_COUNT) so the scheduler and retry logic never pick up logs that have
reached the retry limit.
In
`@src/main/java/com/slam/concertreservation/domain/point/api/PointModuleApi.java`:
- Line 3: Remove the unused import of
com.slam.concertreservation.interfaces.dto.UserPointBalanceResponse from
PointModuleApi to avoid inverting the dependency direction; update the
PointModuleApi interface (class/interface name: PointModuleApi) so it does not
reference types from interfaces.dto — either remove any usages of
UserPointBalanceResponse in the interface methods or replace them with
domain-level types or simple primitives/value objects defined in the domain
layer and perform mapping at the boundary (controller/adapter) instead.
In
`@src/main/java/com/slam/concertreservation/domain/reservation/api/ReservationModuleApi.java`:
- Around line 3-4: Remove the unused imports ReservationCancelResponse and
ReservationConfirmResponse from ReservationModuleApi and eliminate the
dependency from the domain layer to the interfaces.dto package; instead define
domain-level response types or use existing domain value objects inside
ReservationModuleApi (or return primitives/entities) and perform mapping to
interfaces.dto in the interfaces/adapters layer (i.e., move any references to
ReservationCancelResponse/ReservationConfirmResponse out of ReservationModuleApi
and into the controller/mapper).
In
`@src/main/java/com/slam/concertreservation/domain/reservation/application/ReservationModuleFacade.java`:
- Around line 25-27: In ReservationModuleFacade update the two catch blocks that
call ReservationOperationResult.fail(reservationId, e.getMessage()) to guard
against a null message by passing a non-null fallback (e.g., use e.getMessage()
if not null otherwise e.getClass().getSimpleName()); ensure both occurrences are
changed so ReservationOperationResult.fail always receives a non-null string.
In
`@src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/entities/PaymentJpaEntity.java`:
- Around line 9-10: Add an explicit `@Table` annotation to the PaymentJpaEntity
class to specify the intended DB table name (consistent with other entities like
ReservationJpaEntity/UserJpaEntity) and import jakarta.persistence.Table; update
the class declaration for PaymentJpaEntity to include `@Table`(name = "payment")
(or the project’s canonical table name) so Hibernate doesn't default to
payment_jpa_entity.
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`:
- Around line 236-244: The processRefund handler in ConcertReservationController
accepts a path variable paymentId but never uses it; change processRefund to
fetch the original Payment by paymentId (via your
PaymentRepository/PaymentService) and derive userId, price, and reservationId
from that entity before calling
paymentOrchestrator.processRefund(payment.getUserId(), payment.getPrice(),
payment.getReservationId()); alternatively, if you must accept request params,
validate they match the fetched Payment and return 400 on mismatch—update the
method signature and logic in processRefund accordingly and remove or reconcile
the unused request params to ensure the URL paymentId actually controls which
payment is refunded.
In
`@src/main/java/com/slam/concertreservation/interfaces/dto/ReservationConfirmResponse.java`:
- Around line 21-32: The static factory method ReservationConfirmResponse.from
is missing the mapping for seatNumber (so seatNumber stays default 0); update
the builder call in ReservationConfirmResponse.from to set
.seatNumber(reservation.getSeatNumber()) (or convert appropriately if types
differ) alongside the other fields—mirror the fix already used in
ReservationCancelResponse to ensure the DTO carries the actual seat number from
the Reservation entity.
In
`@src/test/java/com/slam/concertreservation/domain/point/application/PointModuleFacadeRetryIntegrationTest.java`:
- Around line 124-136: The test shouldNotRetry_WhenBusinessException currently
has no assertions; update it to assert the operation failed and that the service
was called only once (no retries). After invoking
pointModuleApi.increaseUserPointBalance(userId, amount), assert that the
returned PointOperationResult indicates failure (e.g., isSuccess() is false) and
its error code equals ErrorCode.INVALID_REQUEST, and add Mockito verification
verify(pointService, times(1)).increaseUserPointBalance(userId, amount) to
ensure no retry occurred; use the existing symbols PointOperationResult,
BusinessRuleViolationException, pointService.increaseUserPointBalance, and
pointModuleApi.increaseUserPointBalance.
🧹 Nitpick comments (20)
src/test/java/com/slam/concertreservation/Hhp7ConcertReservationApplicationTests.java (1)
8-8: Unused import ofEnableRetry.
@EnableRetryis imported but never applied to this test class. Since the test loadsHhp7ConcertReservationApplication.class(which already carries@EnableRetry), retry is already enabled in this test context. Consider removing the unused import to keep the file clean.Proposed fix
-import org.springframework.retry.annotation.EnableRetry;src/main/java/com/slam/concertreservation/domain/payment/model/CompensationTxLog.java (2)
86-95:markAsFailed()does not enforceMAX_RETRY_COUNT.
MAX_RETRY_COUNTis defined in this class but never enforced here —retryCountcan be incremented beyond 3 ifmarkAsFailed()is called without external guards. Consider adding a check insidemarkAsFailed()(orincrementRetryCount()) to prevent exceeding the limit, keeping the invariant within the domain model.Proposed fix
public CompensationTxLog markAsFailed() { if(this.status == CompensationTxStatus.COMPLETED) { throw new BusinessRuleViolationException(ErrorCode.INVALID_REQUEST, "이미 완료 처리된 보상 트랜잭션입니다."); } + if(this.retryCount >= MAX_RETRY_COUNT) { + throw new BusinessRuleViolationException(ErrorCode.INVALID_REQUEST, "최대 재시도 횟수를 초과한 보상 트랜잭션입니다."); + } this.status = CompensationTxStatus.FAILED; this.incrementRetryCount(); return this; }
47-58:LocalDateTime.now()increate()hinders testability.Using
LocalDateTime.now()directly couples the domain model to the system clock. Consider accepting aClockorLocalDateTimeparameter to make unit tests deterministic. This is a minor concern given the codebase style.src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/entities/CompensationTxLogJpaEntity.java (1)
22-33:CompensationTxStatus.valueOf()will throwIllegalArgumentExceptionon corrupt data.If the
statuscolumn contains an unexpected string,valueOfthrows an unchecked exception. This is typically acceptable when the write path consistently uses.name(), but consider whether defensive handling is warranted for a compensation log table that may be manually updated during incident response.src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/CompensationTxLogJpaRepository.java (1)
8-25: Consider adding a combined query for the retry scheduler.The compensation retry scheduler likely needs to find logs that are
FAILEDAND haveretryCount < MAX_RETRY_COUNT. Currently this requires either two separate queries or in-memory filtering. A combined derived query would be more efficient.Proposed addition
+ List<CompensationTxLogJpaEntity> findAllByStatusAndRetryCountLessThan( + String status, int maxRetryCount);Also consider adding database indexes on
statusandretryCountcolumns if the compensation log table is expected to grow, as the scheduler will query this table periodically.#!/bin/bash # Check how the scheduler queries compensation logs rg -n 'findAll|compensationTxLog' --type=java -g '*Scheduler*' -g '*RepositoryImpl*' -C 3src/test/java/com/slam/concertreservation/domain/payment/service/CompensationTxLogServiceUnitTest.java (1)
33-36: Prefer@ExtendWith(MockitoExtension.class)over manualopenMocks.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat is never closed here, leaking resources. The idiomatic JUnit 5 approach is to annotate the class with@ExtendWith(MockitoExtension.class)and remove thesetUpmethod entirely.Proposed fix
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class CompensationTxLogServiceUnitTest { `@Mock` private CompensationTxLogRepository compensationTxLogRepository; `@InjectMocks` private CompensationTxLogService compensationTxLogService; - `@BeforeEach` - void setUp() { - MockitoAnnotations.openMocks(this); - }src/main/java/com/slam/concertreservation/domain/payment/application/CompensationTxRetryScheduler.java (1)
19-19: No distributed-lock guard — concurrent scheduler instances may double-process logs.If the application runs with multiple instances, overlapping scheduler invocations could process the same
PENDINGlog simultaneously, leading to duplicate point adjustments. Consider using a distributed lock (e.g.,ShedLock, database advisory lock) or aSELECT … FOR UPDATE SKIP LOCKEDpattern.src/main/java/com/slam/concertreservation/interfaces/dto/ReservationConfirmResponse.java (1)
1-33: Consider extracting a shared base or common DTO.
ReservationConfirmResponseandReservationCancelResponseare structurally identical — same fields, same mapping logic. This duplication increases the surface for bugs like theseatNumberomission. A shared base record or a singleReservationResponsewith thestatusfield differentiating them would reduce maintenance burden.src/test/java/com/slam/concertreservation/domain/payment/service/PaymentServiceUnitTest.java (1)
35-38: Prefer@ExtendWith(MockitoExtension.class)for consistency and correctness.Other test classes in this PR (e.g.,
CompensationTxRetrySchedulerTest) use@ExtendWith(MockitoExtension.class). UsingMockitoAnnotations.openMocks(this)is the older pattern and leaks the returnedAutoCloseablesince it's never closed.Proposed fix
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) class PaymentServiceUnitTest { `@Mock` private PaymentRepository paymentRepository; `@InjectMocks` private PaymentService paymentService; - - `@BeforeEach` - void setUp() { - MockitoAnnotations.openMocks(this); - }src/main/java/com/slam/concertreservation/domain/payment/repository/CompensationTxLogRepository.java (1)
29-29: Naming inconsistency: missing "By" prefix.
findAllCreatedAtBetweendoesn't follow the naming convention used elsewhere (e.g.,PaymentRepository.findAllByCreatedAtBetween). Should befindAllByCreatedAtBetweenfor consistency.Proposed fix
- List<CompensationTxLog> findAllCreatedAtBetween(LocalDateTime startDateTime, LocalDateTime endDateTime); + List<CompensationTxLog> findAllByCreatedAtBetween(LocalDateTime startDateTime, LocalDateTime endDateTime);src/main/java/com/slam/concertreservation/domain/payment/model/Payment.java (1)
52-61: No input validation increate()factory.
createsilently accepts nulluserId, nullreservationId, or negativeprice. Consider adding precondition checks (e.g.,Objects.requireNonNull, price > 0) to fail fast with a clear error rather than persisting invalid data.src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java (1)
251-255: Inconsistent return type: missingResponseEntitywrapper.Every other endpoint in this controller returns
ResponseEntity<...>, butgetPaymentsreturns a rawPage<PaymentProcessResponse>. This inconsistency can affect content negotiation and HTTP status code control.Proposed fix
`@GetMapping`("/payments") -public Page<PaymentProcessResponse> getPayments(`@RequestParam` Long userId, Pageable pageable) { - return paymentService.getPaymentHistoryOfUser(userId, pageable) - .map(PaymentProcessResponse::from); +public ResponseEntity<Page<PaymentProcessResponse>> getPayments(`@RequestParam` Long userId, Pageable pageable) { + Page<PaymentProcessResponse> page = paymentService.getPaymentHistoryOfUser(userId, pageable) + .map(PaymentProcessResponse::from); + return ResponseEntity.ok(page); }src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/impl/PaymentRepositoryImpl.java (1)
3-4: Unused imports:ErrorCodeandUnavailableRequestException.These imports are not referenced anywhere in the class.
Proposed fix
-import com.slam.concertreservation.common.error.ErrorCode; -import com.slam.concertreservation.common.exceptions.UnavailableRequestException;src/main/java/com/slam/concertreservation/domain/payment/service/PaymentService.java (2)
33-39: Inconsistent@Transactionalonfailandrefund— both are missing it.
initiateandcompleteare annotated with@Transactional, butfailandrefundare not, despite all four performing the samepaymentRepository.save(...)operation. This inconsistency could lead to subtle issues if these methods are extended or called in a context where a transaction boundary is expected.Proposed fix
+ `@Transactional` public Payment fail(Payment payment) { return paymentRepository.save(payment.withStatus(PaymentStatus.FAILED)); } + `@Transactional` public Payment refund(Payment payment) { return paymentRepository.save(payment.withStatus(PaymentStatus.REFUNDED)); }
41-67: Read-only queries could benefit from@Transactional(readOnly = true).The query methods (
getPaymentById,getPaymentHistoryOfUser, etc.) perform reads only. Adding@Transactional(readOnly = true)enables Hibernate to skip dirty-checking and can improve performance, especially for paginated queries.src/test/java/com/slam/concertreservation/domain/reservation/application/ReservationModuleFacadeUnitTest.java (1)
25-28:openMocksreturns anAutoCloseablethat is never closed.
MockitoAnnotations.openMocks(this)returns anAutoCloseable. Not closing it can leak mock state between tests. Consider using@ExtendWith(MockitoExtension.class)instead, which handles lifecycle automatically—consistent with idiomatic JUnit 5 + Mockito usage.♻️ Suggested refactor
-import org.mockito.MockitoAnnotations; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class ReservationModuleFacadeUnitTest { `@Mock` private ReservationService reservationService; `@InjectMocks` private ReservationModuleFacade reservationModuleFacade; - `@BeforeEach` - void setUp() { - MockitoAnnotations.openMocks(this); - }src/test/java/com/slam/concertreservation/domain/payment/application/PaymentOrchestratorUnitTest.java (1)
48-51: SameopenMocksleak as in the other test file—prefer@ExtendWith(MockitoExtension.class).See the same suggestion as in
ReservationModuleFacadeUnitTest.src/main/java/com/slam/concertreservation/domain/payment/service/CompensationTxLogService.java (2)
24-32:markAsFailed/markAsCompletedmutate the caller's object as a side effect.These methods call
compensationTxLog.markAsFailed()/markAsCompleted()on the passed-in domain object before saving, so the caller's reference is also mutated regardless of whether the save succeeds. This is a subtle contract that could lead to inconsistent state if the save fails. Consider either documenting this behavior or operating on a copy.
38-41: UsingUnavailableRequestExceptionfor a missing resource feels semantically off.The exception name suggests the request itself is unavailable (e.g., rate-limited), but here it indicates a resource was not found. If a more specific
ResourceNotFoundExceptionexists or is planned, it would be a better fit. Minor naming concern—not blocking.src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/impl/CompensationTxLogRepositoryImpl.java (1)
43-49: Use.name()instead of.toString()for enum-to-string conversion to match the entity's persistence logic.The
fromDomain()method inCompensationTxLogJpaEntityuses.name()to persist the enum. For consistency and clarity, the query should also use.name()rather than.toString(), which is the standard practice for enum-based persistence.- return compensationTxLogJpaRepository.findAllByStatus(compensationTxStatus.toString()) + return compensationTxLogJpaRepository.findAllByStatus(compensationTxStatus.name())
| @Scheduled(fixedDelay = 60000) // 1분 간격 | ||
| public void retryFailedCompensations() { | ||
| List<CompensationTxLog> targets = compensationTxLogService.getAllRetriables(); | ||
|
|
||
| for (CompensationTxLog log : targets) { | ||
| PointOperationResult result; | ||
|
|
||
| // 양수: 포인트 복구(증가), 음수: 포인트 회수(차감) | ||
| if (log.getPrice() > 0) { | ||
| result = pointModuleApi.increaseUserPointBalance(log.getUserId(), log.getPrice()); | ||
| } else { | ||
| result = pointModuleApi.decreaseUserPointBalance(log.getUserId(), Math.abs(log.getPrice())); | ||
| } | ||
|
|
||
| if (result.success()) { | ||
| compensationTxLogService.markAsCompleted(log); | ||
| } else { | ||
| compensationTxLogService.markAsFailed(log); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the CompensationTxLogService implementation
echo "=== Finding CompensationTxLogService ==="
find . -type f -name "CompensationTxLogService.java" | head -5
echo "=== Searching for getAllRetriables method ==="
rg -n "getAllRetriables" --type=java -B2 -A10
echo "=== Searching for markAsFailed method ==="
rg -n "markAsFailed" --type=java -B2 -A10
echo "=== Searching for findAllByStatus usage in service ==="
rg -n "findAllByStatus" --type=java -B2 -A3Repository: leonroars/slam
Length of output: 39115
🏁 Script executed:
#!/bin/bash
# Check the full CompensationTxRetryScheduler implementation
echo "=== Full CompensationTxRetryScheduler.java ==="
cat -n src/main/java/com/slam/concertreservation/domain/payment/application/CompensationTxRetryScheduler.java
echo ""
echo "=== Check if there are other scheduled methods or retry mechanisms ==="
rg -n "@Scheduled|retryCount|FAILED" src/main/java/com/slam/concertreservation/domain/payment --type=java -B1 -A1 | head -40Repository: leonroars/slam
Length of output: 5690
Compensation logs get only one retry — FAILED status is never re-fetched.
getAllRetriables() queries only PENDING logs. When a retry fails, markAsFailed() sets status to FAILED, so the log is never picked up again. This means the scheduler provides a single additional attempt, not continuous retries. For an eventual-consistency mechanism, consider either keeping logs in PENDING with an incremented retry count and a retry cap, or querying both PENDING and FAILED with a configurable max retry limit.
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/CompensationTxRetryScheduler.java`
around lines 19 - 38, retryFailedCompensations() only reprocesses PENDING
because getAllRetriables() excludes FAILED and markAsFailed() flips status to
FAILED, so logs get only one retry; change the flow so retries continue until a
max: either (A) modify compensationTxLogService.getAllRetriables() to return
entries with status PENDING or FAILED whose retryCount < maxRetries
(configurable) and leave markAsFailed() to only increment retryCount and
optionally set status=FAILED only when retryCount >= maxRetries, or (B) keep
getAllRetriables() returning only PENDING but change
markAsFailed(CompensationTxLog) to increment and persist a retryCount and keep
status=PENDING until retryCount exceeds maxRetries then set FAILED; update
CompensationTxLog to track retryCount and ensure retry cap is enforced before
calling markAsCompleted()/final failure.
| if (log.getPrice() > 0) { | ||
| result = pointModuleApi.increaseUserPointBalance(log.getUserId(), log.getPrice()); | ||
| } else { | ||
| result = pointModuleApi.decreaseUserPointBalance(log.getUserId(), Math.abs(log.getPrice())); | ||
| } |
There was a problem hiding this comment.
Edge case: price == 0 falls into the else branch.
If log.getPrice() is 0, it will call decreaseUserPointBalance with amount 0. While likely a no-op, consider guarding with an explicit check or logging a warning, since a zero-price compensation log is likely a bug upstream.
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/CompensationTxRetryScheduler.java`
around lines 27 - 31, In CompensationTxRetryScheduler, guard the price==0 case
so it doesn't fall through to decreaseUserPointBalance with 0: update the method
that currently branches on log.getPrice() (using log.getPrice(),
pointModuleApi.increaseUserPointBalance and
pointModuleApi.decreaseUserPointBalance) to explicitly check for price == 0
first, log a warning or error mentioning the unexpected zero-price compensation
(include identifying info like log.getUserId()/log id), and skip calling the
pointModuleApi in that case; keep the existing increase/decrease branches for
positive/negative prices unchanged.
| import com.slam.concertreservation.domain.payment.service.CompensationTxLogService; | ||
| import com.slam.concertreservation.domain.point.api.PointOperationResult; | ||
| import com.slam.concertreservation.domain.reservation.api.ReservationOperationResult; | ||
| import com.slam.concertreservation.interfaces.dto.PaymentProcessResponse; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Presentation-layer DTO (PaymentProcessResponse) leaks into the domain/application layer.
PaymentOrchestrator lives in domain.payment.application but imports and returns interfaces.dto.PaymentProcessResponse. This couples the domain layer to the presentation layer, violating the layered architecture boundaries this PR establishes for other modules (Point, Reservation expose only api interfaces and DTOs).
The orchestrator should return a domain object (e.g., Payment) and let the controller map to the response DTO.
Proposed approach
-import com.slam.concertreservation.interfaces.dto.PaymentProcessResponse;
...
- public PaymentProcessResponse processPayment(Long userId, int price, Long reservationId) {
+ public Payment processPayment(Long userId, int price, Long reservationId) {
Payment initiatedPayment = paymentService.initiate(userId, price, reservationId);
PointOperationResult deduction = pointModuleApi.decreaseUserPointBalance(userId, price);
if (!deduction.success()) {
- return PaymentProcessResponse.from(paymentService.fail(initiatedPayment));
+ return paymentService.fail(initiatedPayment);
}
// ... similar changes for other return statementsThen in the controller:
- PaymentProcessResponse response = paymentOrchestrator.processPayment(userId, price, reservationId);
- return ResponseEntity.ok(response);
+ Payment payment = paymentOrchestrator.processPayment(userId, price, reservationId);
+ return ResponseEntity.ok(PaymentProcessResponse.from(payment));Also applies to: 26-50
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/PaymentOrchestrator.java`
at line 6, PaymentOrchestrator currently imports and returns the presentation
DTO PaymentProcessResponse, leaking presentation concerns into the
domain/application layer; change the orchestrator to return a domain-level
Payment (or other domain result object) instead of PaymentProcessResponse,
remove the import of
com.slam.concertreservation.interfaces.dto.PaymentProcessResponse from
PaymentOrchestrator, update the orchestrator method signature(s) (e.g.,
processPayment/orchestratePayment) and any internal returns to produce the
domain Payment, and move the mapping to PaymentProcessResponse into the
controller layer so controllers call PaymentOrchestrator to get the domain
Payment and translate it to the DTO for the API response.
| // 4. 결제 확정 | ||
| return PaymentProcessResponse.from(paymentService.complete(initiatedPayment)); |
There was a problem hiding this comment.
No error handling if the final complete/refund step throws.
If paymentService.complete(initiatedPayment) on line 49 (or paymentService.refund on line 75) throws an exception, points have already been deducted and the reservation confirmed, but the payment record stays PENDING. There is no compensation path for this scenario.
Consider wrapping the final persistence step in a try-catch that either retries or logs a compensation entry so the payment record can be reconciled.
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/PaymentOrchestrator.java`
around lines 48 - 49, Wrap the final persistence calls
paymentService.complete(initiatedPayment) and
paymentService.refund(initiatedPayment) in a try-catch inside
PaymentOrchestrator so a thrown exception doesn't leave the system with deducted
points and a PENDING payment; on catch, perform a compensating action such as
retrying the persistence (with a limited retry loop/backoff) or
creating/flagging a reconciliation/compensation record (e.g., via a new
paymentRepository.createCompensationEntry or
paymentService.markForReconciliation) and log the full error and context, then
return an appropriate failure response instead of letting the exception escape;
ensure PaymentProcessResponse.from is only called after the persistence
succeeds.
| public PaymentProcessResponse processRefund(Long userId, int price, Long reservationId) { | ||
| // 1. PENDING 상태의 Payment 생성 | ||
| Payment initiatedRefund = paymentService.initiate(userId, price, reservationId); | ||
|
|
||
| // 2. 포인트 원상 복구 | ||
| PointOperationResult restoration = pointModuleApi.increaseUserPointBalance(userId, price); | ||
| if (!restoration.success()) { | ||
| return PaymentProcessResponse.from(paymentService.fail(initiatedRefund)); | ||
| } | ||
|
|
||
| // 3. 예약 취소 | ||
| ReservationOperationResult cancellation = reservationModuleApi.cancelReservation(reservationId); | ||
| if (!cancellation.success()) { | ||
| compensatePointIncrease( | ||
| initiatedRefund.getUserId(), | ||
| initiatedRefund.getReservationId(), | ||
| initiatedRefund.getPaymentId(), | ||
| initiatedRefund.getPrice()); | ||
|
|
||
| return PaymentProcessResponse.from(paymentService.fail(initiatedRefund)); | ||
| } | ||
|
|
||
| // 4. 환불 확정 | ||
| return PaymentProcessResponse.from(paymentService.refund(initiatedRefund)); | ||
| } |
There was a problem hiding this comment.
Refund flow doesn't validate current reservation/payment state before proceeding.
processRefund accepts userId, price, and reservationId as raw parameters without verifying the reservation is in a refundable state (e.g., CONFIRMED) or that a completed payment exists for it. A caller could trigger a refund for an already-cancelled reservation, causing points to be credited without a valid charge to reverse.
Consider fetching and validating the existing payment/reservation state before initiating the refund.
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/payment/application/PaymentOrchestrator.java`
around lines 52 - 76, Before creating a PENDING refund in processRefund, fetch
and validate the current reservation and payment state: call
reservationModuleApi.getReservation(reservationId) (or equivalent) and verify
it's in a refundable state (e.g., CONFIRMED) and fetch the existing payment via
paymentService.findByReservationId(reservationId) to ensure a completed charge
exists; if validation fails, return a failure response without calling
paymentService.initiate, pointModuleApi.increaseUserPointBalance,
reservationModuleApi.cancelReservation, or paymentService.refund. Keep existing
compensation logic (compensatePointIncrease and paymentService.fail) for
downstream failures but only after the initial pre-checks pass.
| catch (Exception e) { | ||
| return ReservationOperationResult.fail(reservationId, e.getMessage()); | ||
| } |
There was a problem hiding this comment.
e.getMessage() may be null.
Some exceptions (e.g., bare NullPointerException) can have a null message. If ReservationOperationResult.fail doesn't handle null gracefully, this could cause issues downstream. Consider using a fallback:
e.getMessage() != null ? e.getMessage() : e.getClass().getSimpleName()Also applies to: 36-38
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/domain/reservation/application/ReservationModuleFacade.java`
around lines 25 - 27, In ReservationModuleFacade update the two catch blocks
that call ReservationOperationResult.fail(reservationId, e.getMessage()) to
guard against a null message by passing a non-null fallback (e.g., use
e.getMessage() if not null otherwise e.getClass().getSimpleName()); ensure both
occurrences are changed so ReservationOperationResult.fail always receives a
non-null string.
| @Entity | ||
| public class PaymentJpaEntity { |
There was a problem hiding this comment.
Missing @Table annotation — entity will map to an unintended table name.
Other entities in this project (e.g., ReservationJpaEntity, UserJpaEntity) explicitly declare @Table(name = "..."). Without it, Hibernate's default naming strategy will derive the table name from the class name (e.g., payment_jpa_entity), which is almost certainly not the intended table name.
Proposed fix
`@Entity`
+@Table(name = "PAYMENT")
public class PaymentJpaEntity {Also add the import:
import jakarta.persistence.Table;📝 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.
| @Entity | |
| public class PaymentJpaEntity { | |
| import jakarta.persistence.Table; | |
| `@Entity` | |
| `@Table`(name = "PAYMENT") | |
| public class PaymentJpaEntity { |
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/entities/PaymentJpaEntity.java`
around lines 9 - 10, Add an explicit `@Table` annotation to the PaymentJpaEntity
class to specify the intended DB table name (consistent with other entities like
ReservationJpaEntity/UserJpaEntity) and import jakarta.persistence.Table; update
the class declaration for PaymentJpaEntity to include `@Table`(name = "payment")
(or the project’s canonical table name) so Hibernate doesn't default to
payment_jpa_entity.
| @PostMapping("/payments/{paymentId}/refund") | ||
| public ResponseEntity<PaymentProcessResponse> processRefund( | ||
| @PathVariable Long paymentId, | ||
| @RequestParam Long userId, | ||
| @RequestParam Integer price, | ||
| @RequestParam Long reservationId) { | ||
| PaymentProcessResponse response = paymentOrchestrator.processRefund(userId, price, reservationId); | ||
| return ResponseEntity.ok(response); | ||
| } |
There was a problem hiding this comment.
paymentId path variable is accepted but never used.
The endpoint POST /payments/{paymentId}/refund captures paymentId from the path but passes only userId, price, and reservationId to the orchestrator. This means:
- The API contract misleadingly implies a specific payment is being refunded.
- A caller can supply any
paymentIdin the URL with no effect, which is confusing and error-prone.
The paymentId should be used to look up the original payment and derive userId, price, and reservationId from it, or the endpoint design should be reconsidered.
Proposed fix: use paymentId to look up original payment
`@PostMapping`("/payments/{paymentId}/refund")
public ResponseEntity<PaymentProcessResponse> processRefund(
- `@PathVariable` Long paymentId,
- `@RequestParam` Long userId,
- `@RequestParam` Integer price,
- `@RequestParam` Long reservationId) {
- PaymentProcessResponse response = paymentOrchestrator.processRefund(userId, price, reservationId);
+ `@PathVariable` Long paymentId) {
+ Payment originalPayment = paymentService.getPaymentById(paymentId);
+ PaymentProcessResponse response = paymentOrchestrator.processRefund(
+ originalPayment.getUserId(),
+ originalPayment.getPrice(),
+ originalPayment.getReservationId());
return ResponseEntity.ok(response);
}🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`
around lines 236 - 244, The processRefund handler in
ConcertReservationController accepts a path variable paymentId but never uses
it; change processRefund to fetch the original Payment by paymentId (via your
PaymentRepository/PaymentService) and derive userId, price, and reservationId
from that entity before calling
paymentOrchestrator.processRefund(payment.getUserId(), payment.getPrice(),
payment.getReservationId()); alternatively, if you must accept request params,
validate they match the fetched Payment and return 400 on mismatch—update the
method signature and logic in processRefund accordingly and remove or reconcile
the unused request params to ensure the URL paymentId actually controls which
payment is refunded.
| public static ReservationConfirmResponse from(Reservation reservation) { | ||
| return ReservationConfirmResponse.builder() | ||
| .id(String.valueOf(reservation.getId())) | ||
| .userId(String.valueOf(reservation.getUserId())) | ||
| .seatId(String.valueOf(reservation.getSeatId())) | ||
| .concertScheduleId(String.valueOf(reservation.getConcertScheduleId())) | ||
| .price(reservation.getPrice()) | ||
| .status(reservation.getStatus().name()) | ||
| .expiredAt(reservation.getExpiredAt()) | ||
| .createdAt(reservation.getCreatedAt()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
Same seatNumber mapping omission as ReservationCancelResponse.
seatNumber is declared but never set in the builder — it will always be 0.
🐛 Proposed fix
return ReservationConfirmResponse.builder()
.id(String.valueOf(reservation.getId()))
.userId(String.valueOf(reservation.getUserId()))
.seatId(String.valueOf(reservation.getSeatId()))
+ .seatNumber(reservation.getSeatNumber())
.concertScheduleId(String.valueOf(reservation.getConcertScheduleId()))
.price(reservation.getPrice())
.status(reservation.getStatus().name())🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/interfaces/dto/ReservationConfirmResponse.java`
around lines 21 - 32, The static factory method ReservationConfirmResponse.from
is missing the mapping for seatNumber (so seatNumber stays default 0); update
the builder call in ReservationConfirmResponse.from to set
.seatNumber(reservation.getSeatNumber()) (or convert appropriately if types
differ) alongside the other fields—mirror the fix already used in
ReservationCancelResponse to ensure the DTO carries the actual seat number from
the Reservation entity.
| @Test | ||
| @DisplayName("비즈니스 예외는 재시도하지 않는다") | ||
| void shouldNotRetry_WhenBusinessException() { | ||
| // given | ||
| Long userId = 1L; | ||
| int amount = 500; | ||
|
|
||
| when(pointService.increaseUserPointBalance(userId, amount)) | ||
| .thenThrow(new BusinessRuleViolationException(ErrorCode.INVALID_REQUEST, "비즈니스 규칙 위반")); | ||
|
|
||
| // when | ||
| PointOperationResult result = pointModuleApi.increaseUserPointBalance(userId, amount); | ||
| } |
There was a problem hiding this comment.
Test has no assertions — it will always pass regardless of behavior.
shouldNotRetry_WhenBusinessException assigns the result but never asserts on it and never verifies the call count. This means the test passes even if the retry behavior is broken.
🐛 Proposed fix: add assertions matching the decrease counterpart
// when
PointOperationResult result = pointModuleApi.increaseUserPointBalance(userId, amount);
+
+ // then — 비즈니스 실패이므로 1회만 호출
+ assertFalse(result.success());
+ verify(pointService, times(1)).increaseUserPointBalance(userId, amount);
}📝 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.
| @Test | |
| @DisplayName("비즈니스 예외는 재시도하지 않는다") | |
| void shouldNotRetry_WhenBusinessException() { | |
| // given | |
| Long userId = 1L; | |
| int amount = 500; | |
| when(pointService.increaseUserPointBalance(userId, amount)) | |
| .thenThrow(new BusinessRuleViolationException(ErrorCode.INVALID_REQUEST, "비즈니스 규칙 위반")); | |
| // when | |
| PointOperationResult result = pointModuleApi.increaseUserPointBalance(userId, amount); | |
| } | |
| `@Test` | |
| `@DisplayName`("비즈니스 예외는 재시도하지 않는다") | |
| void shouldNotRetry_WhenBusinessException() { | |
| // given | |
| Long userId = 1L; | |
| int amount = 500; | |
| when(pointService.increaseUserPointBalance(userId, amount)) | |
| .thenThrow(new BusinessRuleViolationException(ErrorCode.INVALID_REQUEST, "비즈니스 규칙 위반")); | |
| // when | |
| PointOperationResult result = pointModuleApi.increaseUserPointBalance(userId, amount); | |
| // then — 비즈니스 실패이므로 1회만 호출 | |
| assertFalse(result.success()); | |
| verify(pointService, times(1)).increaseUserPointBalance(userId, amount); | |
| } |
🤖 Prompt for AI Agents
In
`@src/test/java/com/slam/concertreservation/domain/point/application/PointModuleFacadeRetryIntegrationTest.java`
around lines 124 - 136, The test shouldNotRetry_WhenBusinessException currently
has no assertions; update it to assert the operation failed and that the service
was called only once (no retries). After invoking
pointModuleApi.increaseUserPointBalance(userId, amount), assert that the
returned PointOperationResult indicates failure (e.g., isSuccess() is false) and
its error code equals ErrorCode.INVALID_REQUEST, and add Mockito verification
verify(pointService, times(1)).increaseUserPointBalance(userId, amount) to
ensure no retry occurred; use the existing symbols PointOperationResult,
BusinessRuleViolationException, pointService.increaseUserPointBalance, and
pointModuleApi.increaseUserPointBalance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/detailed_docs/en/adr-payment-saga-module-api.md (1)
114-114: Clarify total eventual-consistency window.Line 114 is slightly ambiguous operationally. Consider stating the worst-case total delay explicitly (e.g., “up to ~3 minutes across 3 retry intervals, plus execution time”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/detailed_docs/en/adr-payment-saga-module-api.md` at line 114, Update the “Eventual consistency” sentence about CompensationTxLog to explicitly state the worst-case total delay: mention that with 3 retry attempts at 60 seconds per retry the system can be inconsistent for up to ~180 seconds (≈3 minutes) plus the actual compensation execution time, e.g., replace or augment the existing line referencing CompensationTxLog to read something like “up to ~3 minutes across 3 retry intervals (60s each), plus execution time” so operators clearly see the total eventual-consistency window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/detailed_docs/en/adr-payment-saga-module-api.md`:
- Line 107: The wording on line 107 is redundant; replace "Module API interfaces
serve as explicit contracts." with the tighter phrasing "Module APIs serve as
explicit contracts." and ensure the following sentence ("Cross-domain
dependencies are visible and auditable in the interface definitions.") still
reads fluently—adjust "interface definitions" to "API definitions" if needed so
both sentences consistently use "API" instead of "interface".
- Around line 73-76: The ADR currently contradicts itself about
OptimisticLockingFailureException: the table at line referencing "Excluded
exceptions" lists OptimisticLockingFailureException as non-retryable, while
later paragraphs (around the text referencing retries and transient optimistic
lock conflicts, including mentions near the `@Recover` behavior) treat optimistic
lock failures as transient and recommend retry; pick one consistent policy and
update all occurrences accordingly—either remove
OptimisticLockingFailureException from the excluded list and change the
descriptions around retry/@Recover to mark optimistic locks as transient and
retriable, or keep it excluded and rewrite the later paragraphs (and any
`@Recover` behavior text) to state optimistic lock conflicts are
deterministic/non-retriable; be sure to update the table entry for Excluded
exceptions, the paragraphs discussing transient optimistic lock conflicts, and
any references to `@Recover` to reflect the chosen policy and use the exact symbol
OptimisticLockingFailureException where applicable.
---
Nitpick comments:
In `@docs/detailed_docs/en/adr-payment-saga-module-api.md`:
- Line 114: Update the “Eventual consistency” sentence about CompensationTxLog
to explicitly state the worst-case total delay: mention that with 3 retry
attempts at 60 seconds per retry the system can be inconsistent for up to ~180
seconds (≈3 minutes) plus the actual compensation execution time, e.g., replace
or augment the existing line referencing CompensationTxLog to read something
like “up to ~3 minutes across 3 retry intervals (60s each), plus execution time”
so operators clearly see the total eventual-consistency window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdf3101e-ad06-4a33-8145-74556f301377
📒 Files selected for processing (1)
docs/detailed_docs/en/adr-payment-saga-module-api.md
| | Excluded exceptions | `BusinessRuleViolationException`, `OptimisticLockingFailureException` | | ||
|
|
||
| Non-retryable exceptions (business rule violations, optimistic lock failures) are returned immediately as a failed result via the `@Recover` method, avoiding unnecessary retry on deterministic failures. | ||
|
|
There was a problem hiding this comment.
Retry policy is internally contradictory for optimistic locking failures.
Line 73 excludes OptimisticLockingFailureException from retries, but Line 99 and Line 110 describe optimistic lock conflicts as transient and improved by retry. Please align these sections so the ADR reflects one consistent policy.
Proposed ADR text adjustment
-| Excluded exceptions | `BusinessRuleViolationException`, `OptimisticLockingFailureException` |
+| Excluded exceptions | `BusinessRuleViolationException` |If optimistic locking is intentionally excluded, then Line 99 and Line 110 should be rewritten accordingly.
Also applies to: 99-100, 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/detailed_docs/en/adr-payment-saga-module-api.md` around lines 73 - 76,
The ADR currently contradicts itself about OptimisticLockingFailureException:
the table at line referencing "Excluded exceptions" lists
OptimisticLockingFailureException as non-retryable, while later paragraphs
(around the text referencing retries and transient optimistic lock conflicts,
including mentions near the `@Recover` behavior) treat optimistic lock failures as
transient and recommend retry; pick one consistent policy and update all
occurrences accordingly—either remove OptimisticLockingFailureException from the
excluded list and change the descriptions around retry/@Recover to mark
optimistic locks as transient and retriable, or keep it excluded and rewrite the
later paragraphs (and any `@Recover` behavior text) to state optimistic lock
conflicts are deterministic/non-retriable; be sure to update the table entry for
Excluded exceptions, the paragraphs discussing transient optimistic lock
conflicts, and any references to `@Recover` to reflect the chosen policy and use
the exact symbol OptimisticLockingFailureException where applicable.
|
|
||
| ### Positive | ||
|
|
||
| - **Clear domain boundaries**: Module API interfaces serve as explicit contracts. Cross-domain dependencies are visible and auditable in the interface definitions. |
There was a problem hiding this comment.
Use non-redundant wording for API contracts.
Line 107 can be tightened: “Module APIs serve as explicit contracts” (instead of “Module API interfaces…”).
🧰 Tools
🪛 LanguageTool
[style] ~107-~107: This phrase is redundant (‘I’ stands for ‘interfaces’). Use simply “APIs”.
Context: ... - Clear domain boundaries: Module API interfaces serve as explicit contracts. Cross-doma...
(ACRONYM_TAUTOLOGY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/detailed_docs/en/adr-payment-saga-module-api.md` at line 107, The
wording on line 107 is redundant; replace "Module API interfaces serve as
explicit contracts." with the tighter phrasing "Module APIs serve as explicit
contracts." and ensure the following sentence ("Cross-domain dependencies are
visible and auditable in the interface definitions.") still reads
fluently—adjust "interface definitions" to "API definitions" if needed so both
sentences consistently use "API" instead of "interface".
1 participant
한국어 (Korean)
PR Summary
본 PR은 SLAM(공연 좌석 예매 시스템) 의 핵심 비즈니스 로직인 결제 및 환불 프로세스 구현을 다룹니다.
결제는 포인트 차감 → 예약 확정이 하나의 흐름으로 이루어지지만, 각 도메인이 독립 트랜잭션을 가지므로 중간 단계 실패 시 데이터 불일치가 발생할 수 있습니다. 이 문제를 다음과 같이 해결했습니다.
Key Architectural Decisions
1. Modular Monolith 내 도메인 격리를 위한 트랜잭션 전략
PaymentOrchestrator)에@Transactional을 적용할 경우, 서로 다른 도메인의 DB 트랜잭션이 하나로 묶여 강한 결합이 발생하고, 향후 물리적 분리가 불가능해지는 문제가 있습니다.PointService,ReservationService)가 독자적인 트랜잭션 경계를 가집니다.2. Synchronous Orchestration (동기식 오케스트레이션) 채택
3. 동시성 제어를 위한 Semantic Lock (의미론적 락)
Reservation도메인에PAYMENT_PENDING상태를 도입했습니다. 결제 진입 시 상태를 변경하여 스케줄러가 해당 건을 건너뛰도록 처리함으로써, 별도의 락(Lock) 없이도 데이터 정합성을 확보했습니다.4. Public API & Facade를 통한 경계 강화
api패키지의 인터페이스와 DTO만을 외부에 노출합니다.Facade레이어를 통해 도메인 내부 구현과 예외(Exception)가 외부로 유출되지 않도록 차단했습니다. 이는 모듈 간 참조를 인터페이스에만 의존하게 하여, 추후 물리적 서버 분리 시 코드 수정 비용을 최소화합니다.Reliability & Consistency (안정성 확보)
실패에 대한 보상 처리의 신뢰성 (Resilience)
compensation_tx_log)에 기록하고, 별도 스케줄러가 이를 재시도하여 결과적 일관성(Eventual Consistency) 을 보장합니다.트랜잭션과 재시도(Retry)의 분리
@Retryable을 트랜잭션이 시작되는 Service 레이어가 아닌, Facade 레이어에 배치했습니다.Related Documents
ADR: Cross-Domain Payment Orchestration Strategy(첨부된 상세 설계 문서 참고)English
PR Summary
This PR implements the core payment and refund process for SLAM (Concert Seat Reservation System).
The payment flow involves point deduction → reservation confirmation as a single sequence, but since each domain maintains independent transactions, data inconsistency can occur when an intermediate step fails. This was addressed with the following design decisions:
Key Architectural Decisions
1. Transaction Strategy for Domain Isolation
@Transactionalat the Orchestrator level would merge database transactions from different domains, creating tight coupling and hindering future physical separation.PaymentOrchestratordoes not manage transactions.PointService,ReservationService) maintains its own transaction boundary.2. Synchronous Orchestration
3. Semantic Lock for Concurrency Control
PAYMENT_PENDINGstate in theReservationdomain acting as a Semantic Lock. Upon payment entry, the reservation status is updated, ensuring the scheduler skips these records. This secures data consistency without the need for distributed locks.4. Boundary Enforcement via Public API & Facade
apipackage.Facadelayer prevents internal implementation details and domain-specific exceptions from leaking out. By relying solely on interfaces for inter-module communication, the cost of future physical separation is minimized.Reliability & Consistency
Resilience in Compensation Logic
compensation_tx_log), and a separate scheduler retries them to ensure Eventual Consistency.Separation of Retry and Transaction
@Retryableis applied at the Facade layer, not the Service layer where transactions begin.rollback-onlycannot be reused, placing retries at the Facade ensures that each retry attempt initiates a fresh transaction, improving recovery success rates.Related Documents
ADR: Cross-Domain Payment Orchestration Strategy(Please refer to the attached document for detailed design rationale)Summary by CodeRabbit