feat(s1): application controllers — REST endpoints + CommandHandler (STA-114)#112
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds payment REST endpoints, DTOs, domain exceptions, a PaymentCommandHandler that manages idempotent initiation and cancellation and starts/signals a Temporal workflow, extends global exception handling, and adds unit/MVC tests plus two test dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as "PaymentController"
participant CmdHandler as "PaymentCommandHandler"
participant Repo as "PaymentRepository"
participant WorkflowClient as "WorkflowClient"
participant TemporalWF as "Temporal (PaymentWorkflow)"
Client->>Controller: POST /v1/payments\n(Idempotency-Key, InitiatePaymentRequest)
Controller->>CmdHandler: initiatePayment(idempotencyKey, correlationId, ...)
CmdHandler->>Repo: findByIdempotencyKey(idempotencyKey)
alt replay found
Repo-->>CmdHandler: existing Payment
CmdHandler-->>Controller: InitiateResult(payment, replay=true)
Controller-->>Client: 200 OK
else first-time
Repo-->>CmdHandler: empty
CmdHandler->>Repo: save(new Payment)
Repo-->>CmdHandler: saved Payment
CmdHandler->>WorkflowClient: startWorkflow(payment, ...)
WorkflowClient->>TemporalWF: start(PaymentRequest)
TemporalWF-->>WorkflowClient: started
CmdHandler-->>Controller: InitiateResult(payment, replay=false)
Controller-->>Client: 201 Created
end
sequenceDiagram
actor Client
participant Controller as "PaymentController"
participant CmdHandler as "PaymentCommandHandler"
participant Repo as "PaymentRepository"
participant WorkflowClient as "WorkflowClient"
participant TemporalWF as "Temporal (PaymentWorkflow)"
Client->>Controller: POST /v1/payments/{paymentId}/cancel\n(CancelPaymentRequest)
Controller->>CmdHandler: cancelPayment(paymentId, reason)
CmdHandler->>Repo: findById(paymentId)
alt not found
Repo-->>CmdHandler: empty
CmdHandler-->>Controller: throws PaymentNotFoundException
Controller-->>Client: 404 Not Found
else found
Repo-->>CmdHandler: Payment
alt terminal state
CmdHandler-->>Controller: throws PaymentNotCancellableException
Controller-->>Client: 409 Conflict
else cancellable
CmdHandler->>WorkflowClient: getWorkflowStub(paymentId)
WorkflowClient->>TemporalWF: signal(cancelPayment, CancelRequest)
TemporalWF-->>WorkflowClient: acknowledged
CmdHandler-->>Controller: Payment
Controller-->>Client: 200 OK
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
🚥 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)
Comment |
…(STA-114) Add PaymentController (POST/GET/cancel), PaymentCommandHandler with idempotent replay via Idempotency-Key, Temporal workflow start/cancel signalling, and 14 new tests (192 total unit, 20 IT). Closes LIN-114 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
56c8981 to
8b7b204
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/CancelPaymentRequest.java`:
- Around line 8-11: Add a maximum length constraint to the CancelPaymentRequest
record's reason field to prevent arbitrarily large inputs: update the reason
declaration in CancelPaymentRequest by adding a `@Size`(max =
<appropriate_length>) annotation alongside `@NotBlank` (choose a sensible max,
e.g. 255 or what your DB/logging supports) so validation will reject overly long
cancellation reasons before they reach persistence or logs.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/InitiatePaymentRequest.java`:
- Around line 20-22: The sourceAmount field in InitiatePaymentRequest lacks an
upper bound; add a `@DecimalMax` annotation to enforce a sensible ceiling (or
reference a business-configured constant) and update the validation message
accordingly. Locate the sourceAmount declaration in the InitiatePaymentRequest
record/class and add a `@DecimalMax`(value = "<ceiling>", message = "sourceAmount
must be <= <ceiling>") (or wire the ceiling from configuration/business rules)
so extremely large values are rejected by validation.
- Around line 24-34: Update the InitiatePaymentRequest DTO to validate ISO
formats: add `@Pattern` annotations to sourceCurrency and targetCurrency enforcing
ISO 4217 (e.g., regex "^[A-Z]{3}$" or case-insensitive variant) and to
sourceCountry and targetCountry enforcing ISO 3166-1 alpha-2 (e.g.,
"^[A-Z]{2}$"); include clear validation messages for each field and ensure the
annotations are imported/used on the existing fields (sourceCurrency,
targetCurrency, sourceCountry, targetCountry) in the InitiatePaymentRequest
class, or implement an equivalent custom validator if you need more complex
logic (e.g., checking against a known list of codes).
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.java`:
- Line 42: The current log statement in PaymentController (log.info("POST
/v1/payments idempotencyKey={}, senderId={}", idempotencyKey,
request.senderId())) and the similar one that logs request.reason() must not
emit raw, user-controlled idempotency keys or free-text reasons; update these
log calls to either (a) log stable internal identifiers (e.g., paymentId or a
correlationId) instead of idempotencyKey, or (b) redact or hash the
idempotencyKey/reason before logging (e.g., replace with a fixed token like
"<redacted>" or a one-way hash), and keep senderId or other non-sensitive
metadata as needed; locate the log usage in PaymentController and replace the
offending parameters accordingly (avoid logging request.reason() raw and do not
log idempotencyKey directly).
- Around line 39-41: The Idempotency-Key header parameter in
PaymentController.initiatePayment currently allows blank/whitespace values which
bypass idempotency checks; add a javax/jakarta validation constraint by
annotating the idempotencyKey parameter with `@NotBlank` so Spring method-level
validation rejects empty/whitespace headers, e.g., update the parameter
declaration of PaymentController.initiatePayment to include `@NotBlank` on the
idempotencyKey parameter and ensure the controller can use the framework's
built-in method validation.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentResponse.java`:
- Around line 34-53: PaymentResponse.from currently dereferences
payment.corridor() which can be null when Payment is built via the Lombok
builder; update the mapper to null-guard both corridor accesses (the expressions
that call payment.corridor().sourceCountry() and
payment.corridor().targetCountry()) and return null for those fields when
corridor() is null. Locate the PaymentResponse.from(...) method and replace the
direct corridor().sourceCountry()/targetCountry() calls with ternary-style null
checks (or use Optional.ofNullable(payment.corridor()).map(...).orElse(null)) to
avoid NPEs; alternatively, enforce corridor non-null at the Payment record level
via a compact constructor in Payment if you prefer validation at construction
time.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`:
- Around line 60-65: The service currently logs raw user-controlled values
(idempotencyKey and cancel reasons) in PaymentCommandHandler (see the logging
around paymentRepository.findByIdempotencyKey and the later cancel branch), so
change those log statements to avoid printing raw keys/reasons: replace direct
idempotencyKey/cancel reason with either the internal paymentId/correlationId or
a non-reversible fingerprint (e.g., SHA-256 hex of the idempotencyKey) and
redact/omit cancel reason text; keep existing log contexts like paymentId and
correlationId intact and ensure any new fingerprinting uses a stable helper so
logs remain traceable without exposing raw values.
- Around line 62-85: The idempotency race occurs because
paymentRepository.findByIdempotencyKey() and paymentRepository.save(payment)
allow two concurrent requests to both persist the same idempotencyKey; add a
database-level unique constraint on the idempotencyKey column and modify the
Initiate flow to catch duplicate-key errors from paymentRepository.save(payment)
(or the underlying persistence exception), on duplicate-key reload the existing
Payment via paymentRepository.findByIdempotencyKey(idempotencyKey) and return
InitiateResult(existing, true) without calling startWorkflow; ensure only the
thread that successfully inserted the row calls startWorkflow(saved, ...).
Reference: paymentRepository.findByIdempotencyKey, Payment.initiate,
paymentRepository.save, startWorkflow, InitiateResult.
- Around line 80-82: The code currently calls startWorkflow(saved,
sourceCountry, targetCountry) inside initiatePayment() immediately after
paymentRepository.save(payment), which may start a Temporal workflow before the
surrounding `@Transactional` method commits and risk orphaned workflows on
rollback; refactor so initiatePayment() only saves the entity and publishes a
domain event (e.g., PaymentInitiatedEvent) within the transaction, then move the
WorkflowClient.start() call out of startWorkflow() into a separate event
listener annotated with `@TransactionalEventListener`(phase = AFTER_COMMIT) that
receives the PaymentInitiatedEvent and invokes startWorkflow(savedId,
sourceCountry, targetCountry), or alternatively implement a transactional outbox
that the workflow-starter reads after commit. Ensure
PaymentCommandHandler.save/paymentRepository.save remains inside the transaction
and the actual workflow start happens only from the AFTER_COMMIT listener/outbox
processor.
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.java`:
- Around line 52-139: Add a new test method in the InitiatePayment nested class
(e.g., shouldReturn400ForMissingIdempotencyKey) that performs a POST to
"/v1/payments" with a valid JSON body but omits the "Idempotency-Key" header and
asserts status().isBadRequest() (and optionally the error JSON code if your
error handler produces one). This targets the controller handler that declares
`@RequestHeader`("Idempotency-Key") so Spring will reject requests missing that
header; keep the test alongside shouldReturn201/shouldReturn200 to document the
required header contract.
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java`:
- Around line 65-126: The test currently only asserts persistence; update the
"should create payment, save, and start Temporal workflow" case to verify the
Temporal side effect by asserting that
workflowClient.newWorkflowStub(PaymentWorkflow.class, ...) is invoked and that
the returned workflow stub's start method (e.g., startWorkflow or the actual
starter method on PaymentWorkflow) is called with expected arguments;
conversely, in "should return existing on replay" assert that
workflowClient.newWorkflowStub and any calls on the PaymentWorkflow stub are
never invoked. Locate these checks around handler.initiatePayment in
PaymentCommandHandlerTest and add Mockito verifications
(verify/then(...).should()/should(never())) against workflowClient and the
mocked PaymentWorkflow to enforce the workflow start behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66c722d5-43f7-485b-a200-7d678bd1ccfd
📒 Files selected for processing (13)
payment-orchestrator/payment-orchestrator/build.gradle.ktspayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/CancelPaymentRequest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/GlobalExceptionHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/InitiatePaymentRequest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentResponse.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/model/PaymentNotCancellableException.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/model/PaymentNotFoundException.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.javapayment-orchestrator/payment-orchestrator/src/testFixtures/java/com/stablecoin/payments/orchestrator/fixtures/PaymentFixtures.java
| public record CancelPaymentRequest( | ||
| @NotBlank(message = "reason is required") | ||
| String reason | ||
| ) {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a length constraint on reason.
Without a @Size(max = ...) limit, clients could submit arbitrarily large cancellation reasons, potentially causing DB storage issues or log bloat downstream.
🛡️ Optional: Add max length constraint
public record CancelPaymentRequest(
- `@NotBlank`(message = "reason is required")
+ `@NotBlank`(message = "reason is required")
+ `@Size`(max = 500, message = "reason must not exceed 500 characters")
String reason
) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/CancelPaymentRequest.java`
around lines 8 - 11, Add a maximum length constraint to the CancelPaymentRequest
record's reason field to prevent arbitrarily large inputs: update the reason
declaration in CancelPaymentRequest by adding a `@Size`(max =
<appropriate_length>) annotation alongside `@NotBlank` (choose a sensible max,
e.g. 255 or what your DB/logging supports) so validation will reject overly long
cancellation reasons before they reach persistence or logs.
| @NotNull(message = "sourceAmount is required") | ||
| @DecimalMin(value = "0.01", message = "sourceAmount must be positive") | ||
| BigDecimal sourceAmount, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding an upper bound on sourceAmount.
Without @DecimalMax, clients could submit astronomically large values. Business rules should define a sensible ceiling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/InitiatePaymentRequest.java`
around lines 20 - 22, The sourceAmount field in InitiatePaymentRequest lacks an
upper bound; add a `@DecimalMax` annotation to enforce a sensible ceiling (or
reference a business-configured constant) and update the validation message
accordingly. Locate the sourceAmount declaration in the InitiatePaymentRequest
record/class and add a `@DecimalMax`(value = "<ceiling>", message = "sourceAmount
must be <= <ceiling>") (or wire the ceiling from configuration/business rules)
so extremely large values are rejected by validation.
| @NotBlank(message = "sourceCurrency is required") | ||
| String sourceCurrency, | ||
|
|
||
| @NotBlank(message = "targetCurrency is required") | ||
| String targetCurrency, | ||
|
|
||
| @NotBlank(message = "sourceCountry is required") | ||
| String sourceCountry, | ||
|
|
||
| @NotBlank(message = "targetCountry is required") | ||
| String targetCountry |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider ISO validation for currency and country codes.
sourceCurrency/targetCurrency should ideally conform to ISO 4217 (3-letter codes), and country codes to ISO 3166-1 alpha-2. A @Pattern annotation or custom validator could enforce this at the edge.
💡 Example: Pattern validation for currency codes
- `@NotBlank`(message = "sourceCurrency is required")
+ `@NotBlank`(message = "sourceCurrency is required")
+ `@Pattern`(regexp = "^[A-Z]{3}$", message = "sourceCurrency must be a valid 3-letter currency code")
String sourceCurrency,📝 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.
| @NotBlank(message = "sourceCurrency is required") | |
| String sourceCurrency, | |
| @NotBlank(message = "targetCurrency is required") | |
| String targetCurrency, | |
| @NotBlank(message = "sourceCountry is required") | |
| String sourceCountry, | |
| @NotBlank(message = "targetCountry is required") | |
| String targetCountry | |
| `@NotBlank`(message = "sourceCurrency is required") | |
| `@Pattern`(regexp = "^[A-Z]{3}$", message = "sourceCurrency must be a valid 3-letter currency code") | |
| String sourceCurrency, | |
| `@NotBlank`(message = "targetCurrency is required") | |
| String targetCurrency, | |
| `@NotBlank`(message = "sourceCountry is required") | |
| String sourceCountry, | |
| `@NotBlank`(message = "targetCountry is required") | |
| String targetCountry |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/InitiatePaymentRequest.java`
around lines 24 - 34, Update the InitiatePaymentRequest DTO to validate ISO
formats: add `@Pattern` annotations to sourceCurrency and targetCurrency enforcing
ISO 4217 (e.g., regex "^[A-Z]{3}$" or case-insensitive variant) and to
sourceCountry and targetCountry enforcing ISO 3166-1 alpha-2 (e.g.,
"^[A-Z]{2}$"); include clear validation messages for each field and ensure the
annotations are imported/used on the existing fields (sourceCurrency,
targetCurrency, sourceCountry, targetCountry) in the InitiatePaymentRequest
class, or implement an equivalent custom validator if you need more complex
logic (e.g., checking against a known list of codes).
| public ResponseEntity<PaymentResponse> initiatePayment( | ||
| @RequestHeader("Idempotency-Key") String idempotencyKey, | ||
| @Valid @RequestBody InitiatePaymentRequest request) { | ||
| log.info("POST /v1/payments idempotencyKey={}, senderId={}", idempotencyKey, request.senderId()); |
There was a problem hiding this comment.
Stop logging raw idempotency keys and cancel reasons.
Both values are user-controlled. Idempotency-Key is a replay token, and reason is free text that can carry PII or secrets. Log stable identifiers like paymentId/correlation metadata instead, or redact/hash before emission.
Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.java`
at line 42, The current log statement in PaymentController (log.info("POST
/v1/payments idempotencyKey={}, senderId={}", idempotencyKey,
request.senderId())) and the similar one that logs request.reason() must not
emit raw, user-controlled idempotency keys or free-text reasons; update these
log calls to either (a) log stable internal identifiers (e.g., paymentId or a
correlationId) instead of idempotencyKey, or (b) redact or hash the
idempotencyKey/reason before logging (e.g., replace with a fixed token like
"<redacted>" or a one-way hash), and keep senderId or other non-sensitive
metadata as needed; locate the log usage in PaymentController and replace the
offending parameters accordingly (avoid logging request.reason() raw and do not
log idempotencyKey directly).
| log.info("Initiating payment idempotencyKey={}, correlationId={}", idempotencyKey, correlationId); | ||
|
|
||
| Optional<Payment> existing = paymentRepository.findByIdempotencyKey(idempotencyKey); | ||
| if (existing.isPresent()) { | ||
| log.info("Idempotent replay for idempotencyKey={}, paymentId={}", | ||
| idempotencyKey, existing.get().paymentId()); |
There was a problem hiding this comment.
Remove raw idempotency keys and cancel reasons from service logs.
These are user-controlled values and should not be written verbatim at INFO level. Keep paymentId/correlation identifiers, or emit a redacted/hash form if the key must be traceable.
Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`
around lines 60 - 65, The service currently logs raw user-controlled values
(idempotencyKey and cancel reasons) in PaymentCommandHandler (see the logging
around paymentRepository.findByIdempotencyKey and the later cancel branch), so
change those log statements to avoid printing raw keys/reasons: replace direct
idempotencyKey/cancel reason with either the internal paymentId/correlationId or
a non-reversible fingerprint (e.g., SHA-256 hex of the idempotencyKey) and
redact/omit cancel reason text; keep existing log contexts like paymentId and
correlationId intact and ensure any new fingerprinting uses a stable helper so
logs remain traceable without exposing raw values.
| var saved = paymentRepository.save(payment); | ||
|
|
||
| startWorkflow(saved, sourceCountry, targetCountry); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Spring Framework, when does a @transactional method commit relative to method return, and what patterns are recommended to avoid calling external systems before the transaction successfully commits?
💡 Result:
When does a @Transactional method commit (relative to return)?
In Spring’s imperative (thread-bound) transaction model, the transaction is committed after the transactional method’s invocation completes successfully—i.e., the interceptor runs its “commit after returning” step after your method body finishes, and before control is finally returned to the caller. If commit fails, the exception is raised instead of a “successful” return. [1]
How to avoid calling external systems before the transaction is safely committed
Recommended patterns are all about deferring side effects until “after commit”:
-
Register an after-commit callback (low-level, explicit)
- Use Spring transaction synchronizations and perform the external call in an
afterCommit()callback (only runs if commit actually happens). [2]
- Use Spring transaction synchronizations and perform the external call in an
-
Use
@TransactionalEventListener(phase = AFTER_COMMIT)(high-level, idiomatic Spring)- Publish a domain/application event inside the transaction, and handle it with
@TransactionalEventListenerin theAFTER_COMMITphase (the default phase isAFTER_COMMIT). This ensures the handler runs only after a successful commit. [3] - Commonly combined with
@Asyncso the post-commit work doesn’t block the request thread.
- Publish a domain/application event inside the transaction, and handle it with
-
Transactional Outbox (most robust for reliable external publication)
- Persist an “outbox” record (what you intend to send) in the same DB transaction, then have a separate process/thread publish it to the external system and mark it sent. This avoids the “DB committed but external call didn’t happen” / “external call happened but DB rolled back” mismatch. [4]
Sources: TransactionAspectSupport Javadoc (commit-after-returning) [1], TransactionSynchronization Javadoc (afterCommit) [2], Spring Framework Data Access reference (@TransactionalEventListener phases / default) [3], Spring blog on outbox strategies [4].
🏁 Script executed:
find . -name "PaymentCommandHandler.java" -type fRepository: Puneethkumarck/stablebridge-platform
Length of output: 216
🏁 Script executed:
cat -n ./payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java | head -160Repository: Puneethkumarck/stablebridge-platform
Length of output: 7518
Defer Temporal workflow initiation until after database transaction commits.
WorkflowClient.start() executes before the @Transactional method commits. If the transaction rolls back post-workflow-start, you create an orphan workflow for a payment that never persisted. Use Spring's @TransactionalEventListener(phase = AFTER_COMMIT) to publish a domain event inside the transaction, then start the workflow in the event handler—or adopt a transactional outbox pattern for reliable async handoff.
This applies to both initiatePayment() (lines 80–82) and startWorkflow() (lines 125–148).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`
around lines 80 - 82, The code currently calls startWorkflow(saved,
sourceCountry, targetCountry) inside initiatePayment() immediately after
paymentRepository.save(payment), which may start a Temporal workflow before the
surrounding `@Transactional` method commits and risk orphaned workflows on
rollback; refactor so initiatePayment() only saves the entity and publishes a
domain event (e.g., PaymentInitiatedEvent) within the transaction, then move the
WorkflowClient.start() call out of startWorkflow() into a separate event
listener annotated with `@TransactionalEventListener`(phase = AFTER_COMMIT) that
receives the PaymentInitiatedEvent and invokes startWorkflow(savedId,
sourceCountry, targetCountry), or alternatively implement a transactional outbox
that the workflow-starter reads after commit. Ensure
PaymentCommandHandler.save/paymentRepository.save remains inside the transaction
and the actual workflow start happens only from the AFTER_COMMIT listener/outbox
processor.
| @Nested | ||
| @DisplayName("POST /v1/payments") | ||
| class InitiatePayment { | ||
|
|
||
| @Test | ||
| @DisplayName("should return 201 Created for new payment") | ||
| void shouldReturn201() throws Exception { | ||
| // given | ||
| var result = anInitiateResult(); | ||
| given(commandHandler.initiatePayment( | ||
| eq(IDEMPOTENCY_KEY), any(UUID.class), | ||
| eq(SENDER_ID), eq(RECIPIENT_ID), eq(SOURCE_AMOUNT_VALUE), | ||
| eq(SOURCE_CURRENCY), eq(TARGET_CURRENCY), | ||
| eq(SOURCE_COUNTRY), eq(TARGET_COUNTRY))) | ||
| .willReturn(result); | ||
|
|
||
| // when/then | ||
| mockMvc.perform(post("/v1/payments") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", IDEMPOTENCY_KEY) | ||
| .content(""" | ||
| { | ||
| "senderId": "%s", | ||
| "recipientId": "%s", | ||
| "sourceAmount": %s, | ||
| "sourceCurrency": "%s", | ||
| "targetCurrency": "%s", | ||
| "sourceCountry": "%s", | ||
| "targetCountry": "%s" | ||
| } | ||
| """.formatted(SENDER_ID, RECIPIENT_ID, SOURCE_AMOUNT_VALUE, | ||
| SOURCE_CURRENCY, TARGET_CURRENCY, | ||
| SOURCE_COUNTRY, TARGET_COUNTRY))) | ||
| .andExpect(status().isCreated()) | ||
| .andExpect(jsonPath("$.paymentId").exists()) | ||
| .andExpect(jsonPath("$.state").value("INITIATED")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return 200 OK for idempotent replay") | ||
| void shouldReturn200ForReplay() throws Exception { | ||
| // given | ||
| var result = anIdempotentReplayResult(); | ||
| given(commandHandler.initiatePayment( | ||
| eq(IDEMPOTENCY_KEY), any(UUID.class), | ||
| eq(SENDER_ID), eq(RECIPIENT_ID), eq(SOURCE_AMOUNT_VALUE), | ||
| eq(SOURCE_CURRENCY), eq(TARGET_CURRENCY), | ||
| eq(SOURCE_COUNTRY), eq(TARGET_COUNTRY))) | ||
| .willReturn(result); | ||
|
|
||
| // when/then | ||
| mockMvc.perform(post("/v1/payments") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", IDEMPOTENCY_KEY) | ||
| .content(""" | ||
| { | ||
| "senderId": "%s", | ||
| "recipientId": "%s", | ||
| "sourceAmount": %s, | ||
| "sourceCurrency": "%s", | ||
| "targetCurrency": "%s", | ||
| "sourceCountry": "%s", | ||
| "targetCountry": "%s" | ||
| } | ||
| """.formatted(SENDER_ID, RECIPIENT_ID, SOURCE_AMOUNT_VALUE, | ||
| SOURCE_CURRENCY, TARGET_CURRENCY, | ||
| SOURCE_COUNTRY, TARGET_COUNTRY))) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.paymentId").exists()) | ||
| .andExpect(jsonPath("$.state").value("INITIATED")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return 400 Bad Request for invalid request body") | ||
| void shouldReturn400ForInvalidBody() throws Exception { | ||
| // when/then | ||
| mockMvc.perform(post("/v1/payments") | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", IDEMPOTENCY_KEY) | ||
| .content(""" | ||
| { | ||
| "sourceAmount": -1 | ||
| } | ||
| """)) | ||
| .andExpect(status().isBadRequest()) | ||
| .andExpect(jsonPath("$.code").value("PO-0001")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Idempotency-Key is required in PaymentController
ast-grep --pattern $'@RequestHeader($$$"Idempotency-Key"$$$) String $$$'Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
# Find PaymentController
fd -t f "PaymentController.java" -x cat {}Repository: Puneethkumarck/stablebridge-platform
Length of output: 3180
🏁 Script executed:
# Search for Idempotency-Key handling across the codebase
rg -i "idempotency" -A 3 -B 3Repository: Puneethkumarck/stablebridge-platform
Length of output: 50393
🏁 Script executed:
# Look at the test file to understand its imports and setup
cat payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.java | head -60Repository: Puneethkumarck/stablebridge-platform
Length of output: 3238
🏁 Script executed:
# Check total lines in test file and search for additional test methods
wc -l payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 229
🏁 Script executed:
# Search for test methods related to missing header
rg -i "missing.*header|header.*missing|idempotency" payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 717
🏁 Script executed:
# Check if there are any tests in other nested classes or if more tests follow line 139
tail -50 payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2335
Add test for missing Idempotency-Key header on POST /v1/payments.
The controller enforces @RequestHeader("Idempotency-Key") at the method parameter level, which Spring will reject with 400 BAD_REQUEST if absent. No test currently documents this behavior. While validation errors (invalid body) are covered, the required header enforcement should have an explicit test to clarify the contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.java`
around lines 52 - 139, Add a new test method in the InitiatePayment nested class
(e.g., shouldReturn400ForMissingIdempotencyKey) that performs a POST to
"/v1/payments" with a valid JSON body but omits the "Idempotency-Key" header and
asserts status().isBadRequest() (and optionally the error JSON code if your
error handler produces one). This targets the controller handler that declares
`@RequestHeader`("Idempotency-Key") so Spring will reject requests missing that
header; keep the test alongside shouldReturn201/shouldReturn200 to document the
required header contract.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`:
- Around line 105-123: The cancelPayment method in PaymentCommandHandler only
reads the Payment via paymentRepository.findById and signals the Temporal
workflow (cancelPayment on PaymentWorkflow) without persisting changes; mark
this method as read-only by annotating cancelPayment with
`@Transactional`(readOnly = true) to clarify intent and allow read-replica
routing, or if you expect future local state updates leave it unchanged and add
a TODO comment; ensure PaymentNotFoundException and
PaymentNotCancellableException behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fad9bb15-7c5f-4244-8995-23c10f157cc2
📒 Files selected for processing (13)
payment-orchestrator/payment-orchestrator/build.gradle.ktspayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/CancelPaymentRequest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/GlobalExceptionHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/InitiatePaymentRequest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentResponse.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/model/PaymentNotCancellableException.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/model/PaymentNotFoundException.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerMvcTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/application/controller/PaymentControllerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.javapayment-orchestrator/payment-orchestrator/src/testFixtures/java/com/stablecoin/payments/orchestrator/fixtures/PaymentFixtures.java
| public Payment cancelPayment(UUID paymentId, String reason) { | ||
| log.info("Cancelling payment paymentId={}, reason={}", paymentId, reason); | ||
|
|
||
| var payment = paymentRepository.findById(paymentId) | ||
| .orElseThrow(() -> new PaymentNotFoundException(paymentId)); | ||
|
|
||
| if (payment.isTerminal()) { | ||
| throw new PaymentNotCancellableException(paymentId, payment.state()); | ||
| } | ||
|
|
||
| var workflowStub = workflowClient.newWorkflowStub( | ||
| PaymentWorkflow.class, | ||
| "payment-" + paymentId | ||
| ); | ||
| workflowStub.cancelPayment(new CancelRequest(paymentId, reason, "API")); | ||
|
|
||
| log.info("Cancel signal sent for payment paymentId={}", paymentId); | ||
| return payment; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider marking cancelPayment as read-only or restructuring.
cancelPayment retrieves a payment but does not persist any state change—the actual state transition happens within the Temporal workflow. Declaring @Transactional(readOnly = true) clarifies intent and enables DB read-replica routing if applicable. Alternatively, if future iterations update state here, disregard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`
around lines 105 - 123, The cancelPayment method in PaymentCommandHandler only
reads the Payment via paymentRepository.findById and signals the Temporal
workflow (cancelPayment on PaymentWorkflow) without persisting changes; mark
this method as read-only by annotating cancelPayment with
`@Transactional`(readOnly = true) to clarify intent and allow read-replica
routing, or if you expect future local state updates leave it unchanged and add
a TODO comment; ensure PaymentNotFoundException and
PaymentNotCancellableException behavior remains unchanged.
…ge (STA-114) - Handle concurrent duplicate idempotencyKey via DataIntegrityViolationException catch on save — reload existing payment without starting workflow - Add @notblank validation on Idempotency-Key header for proper 400 responses - Add HandlerMethodValidationException handler for Spring MVC 7 method-level parameter validation - Add Temporal workflow start/no-start verification in CommandHandler tests - Add concurrent duplicate race condition test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java (2)
81-91:⚠️ Potential issue | 🔴 CriticalDo not start Temporal from inside this transaction.
Line 83 is not a durable idempotency barrier, but Line 156 performs the external side effect immediately. If this repository is JPA-backed,
save(...)commonly defers the insert/unique-constraint check until flush or commit, so two concurrent requests can still both reachWorkflowClient.start(...), and any later rollback leaves an orphan workflow. Persist/flush the row first, then hand off workflow start inAFTER_COMMITor via an outbox.In Spring Data JPA, does Repository.save(...) execute the SQL insert and surface unique-constraint violations before transaction commit, or is saveAndFlush()/EntityManager.flush() required? Also, what happens if a DataIntegrityViolationException is caught inside a `@Transactional` method after the transaction has been marked rollback-only?Also applies to: 134-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java` around lines 81 - 91, The code starts a Temporal workflow (startWorkflow) while still inside the transaction that calls paymentRepository.save, risking orphan workflows because save may not flush DB constraints; change to persist-and-flush the Payment (use paymentRepository.save + paymentRepository.flush or EntityManager.flush) to ensure the unique constraint is enforced, and move workflow startup out of the transactional commit by invoking startWorkflow only in an AFTER_COMMIT hook (TransactionSynchronizationManager/TransactionSynchronization or an `@TransactionalEventListener`) or by publishing an outbox/event that a separate process consumes; keep the existing DataIntegrityViolationException handling around save/flush and still return InitiateResult(concurrent, true) on duplicate.
61-66:⚠️ Potential issue | 🟠 MajorRedact idempotency keys and cancel reasons from INFO logs.
These are user-controlled values and should not be written verbatim at INFO level. Keep
paymentId/correlationId, or log a stable one-way fingerprint for the idempotency key if replay tracing is required.Also applies to: 85-85, 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java` around lines 61 - 66, The INFO logs in PaymentCommandHandler leak user-controlled idempotency keys and cancel reasons; change the logging at the calls around paymentRepository.findByIdempotencyKey and other log.info usages in this class to avoid printing raw idempotencyKey or cancelReason — either omit them and retain paymentId/correlationId, or compute and log a stable one-way fingerprint (e.g., SHA-256 hex) of the idempotencyKey instead; ensure the idempotencyKey variable is never directly interpolated in log.info calls and apply the same redaction/fingerprinting pattern to the other occurrences referenced in this class (e.g., the idempotency replay log and cancel-reason log sites).payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.java (1)
44-45:⚠️ Potential issue | 🟠 MajorStop logging raw replay tokens and free-text cancel reasons.
Idempotency-Keyis a user-controlled replay token, andreasonis arbitrary text that can carry PII or secrets. Log stable internal identifiers instead, or redact/hash these values before emission.Suggested change
- log.info("POST /v1/payments idempotencyKey={}, senderId={}", idempotencyKey, request.senderId()); + log.info("POST /v1/payments senderId={}", request.senderId()); ... - log.info("POST /v1/payments/{}/cancel reason={}", paymentId, request.reason()); + log.info("POST /v1/payments/{}/cancel", paymentId);Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.java` around lines 44 - 45, The logs currently emit user-controlled sensitive strings (idempotencyKey and free-text cancel reasons); update PaymentController to stop logging raw values by hashing or redacting them before emission (e.g., add a private helper like hashToken(String) or use a shared util to compute a SHA-256/HMAC and log a truncated hex fingerprint) and replace direct uses of idempotencyKey and the cancel reason in the log.info calls with the hashed/truncated fingerprint or with stable internal identifiers (e.g., paymentId) so both the POST log (log.info("POST /v1/payments...")) and the cancel log (the calls around request.cancelReason()) only emit non-sensitive fingerprints or internal IDs. Ensure the helper is deterministic, fast, and documented in code comments.payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java (1)
83-104:⚠️ Potential issue | 🟡 MinorAssert the workflow start, not just stub creation.
This still passes if
PaymentCommandHandler.initiatePayment(...)stops invoking the starter method onworkflowStubafternewWorkflowStub(...). Add a verification on the actual workflow method call so the create-path test covers the Temporal side effect end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java` around lines 83 - 104, The test only verifies newWorkflowStub was created but not that the workflow was actually started; update the test for PaymentCommandHandler.initiatePayment to also verify the workflow starter method on the mocked workflowStub was invoked (e.g., verify that workflowStub.startPayment(...) or whatever the PaymentWorkflow starter method is called) with the expected arguments (idempotency/correlation/payment details), using then(workflowStub).should() (or equivalent) so the Temporal side-effect is asserted end-to-end in addition to newWorkflowStub invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java`:
- Line 12: Remove the unused import of PaymentRequest from the
PaymentCommandHandlerTest file: delete the line importing
com.stablecoin.payments.orchestrator.domain.workflow.dto.PaymentRequest so that
the class PaymentCommandHandlerTest no longer contains an unused import and
Spotless/CI will pass; ensure no other references to PaymentRequest remain in
PaymentCommandHandlerTest before committing.
---
Duplicate comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.java`:
- Around line 44-45: The logs currently emit user-controlled sensitive strings
(idempotencyKey and free-text cancel reasons); update PaymentController to stop
logging raw values by hashing or redacting them before emission (e.g., add a
private helper like hashToken(String) or use a shared util to compute a
SHA-256/HMAC and log a truncated hex fingerprint) and replace direct uses of
idempotencyKey and the cancel reason in the log.info calls with the
hashed/truncated fingerprint or with stable internal identifiers (e.g.,
paymentId) so both the POST log (log.info("POST /v1/payments...")) and the
cancel log (the calls around request.cancelReason()) only emit non-sensitive
fingerprints or internal IDs. Ensure the helper is deterministic, fast, and
documented in code comments.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.java`:
- Around line 81-91: The code starts a Temporal workflow (startWorkflow) while
still inside the transaction that calls paymentRepository.save, risking orphan
workflows because save may not flush DB constraints; change to persist-and-flush
the Payment (use paymentRepository.save + paymentRepository.flush or
EntityManager.flush) to ensure the unique constraint is enforced, and move
workflow startup out of the transactional commit by invoking startWorkflow only
in an AFTER_COMMIT hook
(TransactionSynchronizationManager/TransactionSynchronization or an
`@TransactionalEventListener`) or by publishing an outbox/event that a separate
process consumes; keep the existing DataIntegrityViolationException handling
around save/flush and still return InitiateResult(concurrent, true) on
duplicate.
- Around line 61-66: The INFO logs in PaymentCommandHandler leak user-controlled
idempotency keys and cancel reasons; change the logging at the calls around
paymentRepository.findByIdempotencyKey and other log.info usages in this class
to avoid printing raw idempotencyKey or cancelReason — either omit them and
retain paymentId/correlationId, or compute and log a stable one-way fingerprint
(e.g., SHA-256 hex) of the idempotencyKey instead; ensure the idempotencyKey
variable is never directly interpolated in log.info calls and apply the same
redaction/fingerprinting pattern to the other occurrences referenced in this
class (e.g., the idempotency replay log and cancel-reason log sites).
In
`@payment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java`:
- Around line 83-104: The test only verifies newWorkflowStub was created but not
that the workflow was actually started; update the test for
PaymentCommandHandler.initiatePayment to also verify the workflow starter method
on the mocked workflowStub was invoked (e.g., verify that
workflowStub.startPayment(...) or whatever the PaymentWorkflow starter method is
called) with the expected arguments (idempotency/correlation/payment details),
using then(workflowStub).should() (or equivalent) so the Temporal side-effect is
asserted end-to-end in addition to newWorkflowStub invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f0e80fc-3817-42cb-8d63-e405037a903c
📒 Files selected for processing (4)
payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/GlobalExceptionHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/PaymentController.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandler.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.java
Summary
PaymentController— thin REST handler: POST/v1/payments(201/200 idempotent), GET/v1/payments/{id}, POST/v1/payments/{id}/cancelPaymentCommandHandler— domain service: creates Payment aggregate, saves via repository, starts Temporal workflow, sends cancel signalPaymentResponseDTO with staticfrom(Payment)mapper,InitiatePaymentRequest/CancelPaymentRequestwith Jakarta validationPaymentNotFoundException(404, PO-2001),PaymentNotCancellableException(409, PO-2002) + GlobalExceptionHandler updatesTest plan
Closes STA-114
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Error Handling
Tests
Chores