How I review code

issue (blocking)This writes full card numbers to the log.

PaymentView is a record, so its toString prints every component, cardNumber included. A log is storage like any other under PCI DSS, and it is copied to places we do not control. Log the payment id; and give PaymentView the last four digits instead of the number, so that nothing downstream can leak it either.

issue (blocking)A retried request refunds twice.

A client that times out and sends this POST again gets a second refund, and so does a proxy that replays it. Accept an Idempotency-Key header, store it with the refund under a unique constraint, and answer a repeat with the first refund.

nitpick (non-blocking)30 what?

Duration.ofDays(30), or a configuration property, says the unit, and lets support change the window without a release.

issue (blocking)Money as double.

Binary floating point cannot hold most cents exactly, so sums of refunds drift, and the comparison below can let a cent through or refuse a valid refund. Use BigDecimal at the currency’s scale, or a long of minor units, here, in Refund, in PaymentView, and in the migration, where DOUBLE PRECISION has the same problem.

issueThe window depends on the server’s time zone.

LocalDateTime.now() reads the JVM’s zone, so the cut-off moves when the container’s zone does, and around a daylight-saving change a day is not 24 hours. Keep capturedAt as an Instant and compare with an injected Clock: the second test can then fix the time instead of building a payment 31 days old.

issue (blocking)Two refunds at the same moment can both pass this check.

The sum is read, compared and written in separate steps, so two requests for 60 on a payment of 100 both see nothing refunded and both succeed. Make the check and the write one step: lock the payment row in the transaction (@Lock(PESSIMISTIC_WRITE) on the query), or keep refunded_amount on the payment and update it WHERE refunded_amount + :amount <= amount, checking that one row changed.

issue (blocking)@Transactional does nothing on this call.

Spring applies it through a proxy, and a call from inside the class never passes through the proxy. So issue runs without a transaction, and markSucceeded() changes an object that nobody saves. Make refund the transactional method, or move issue to a bean of its own.

suggestionKeep the provider call out of the database transaction.

Once the transaction works, it holds a connection and row locks for as long as the provider takes to answer, and a rollback after the provider refunded leaves the customer’s money returned with no record of it. Commit the refund as PENDING, then call the provider; an outbox makes that call survive a crash.

questionWhat happens if the provider refunds and this process dies before markSucceeded()?

The customer has the money and we have a PENDING refund, or none at all. Does anything pick up refunds stuck in PENDING and ask the provider? The reconciler only reads succeeded ones.

issueOne query per refund.

Each refund loads its payment separately, so a night with ten thousand refunds makes ten thousand round trips. Load the payments with the refunds, in one join or one findAllById; and if the provider has a refund report, ask for it once instead of once per refund.

issueBigDecimal.equals compares the scale too.

new BigDecimal("10.0").equals(new BigDecimal("10.00")) is false, so a provider that sends two decimals flags every refund as a mismatch. Compare with compareTo(...) != 0, once the amounts are no longer double.

issue (blocking)A timeout is not a failure: retrying this POST can refund twice at the provider.

After a timeout we do not know whether the provider refunded. Sending the request again asks for a second refund. Send our refund id as the provider’s idempotency key, so that a retry returns the first refund instead of making another.

suggestionBack off, with jitter.

Every client caught by the same outage retries on the same one-second beat, so the provider gets its load back in waves just as it recovers. Exponential backoff with full jitter spreads the retries out; the lab’s Charged twice experiment shows the difference.

issue (blocking)This migration fails on any database that has payments.

A NOT NULL column without a default cannot be added to a table that has rows, so the deploy stops here in production and nowhere else. Add it with DEFAULT 0, which PostgreSQL 11 and later apply without rewriting the table, or add it nullable, backfill it, then set NOT NULL.

praiseThese tests read like the refund rules.

Each name states a rule, each test checks one behaviour, and the second refund in the first test is exactly the case that matters. A test with two refunds at the same moment would pin down the race in RefundService.