feat(infra): implement real idempotency storage and replay for S10/S11/S13 (STA-72)#185
Merged
Merged
Conversation
…1/S13 (STA-72)
Replace validation-only IdempotencyKeyFilter in api-gateway-iam (S10),
merchant-onboarding (S11), and merchant-iam (S13) with full idempotency
implementation that persists key + SHA-256 request hash + response body,
replays cached responses on duplicate requests, and returns 422 on
hash mismatches.
Changes per service:
- Flyway migration to create {prefix}_idempotency_keys table
- IdempotencyKeyFilter rewritten with JdbcTemplate persistence,
CachedBodyRequestWrapper for body re-reading, and
ContentCachingResponseWrapper for response capture
- IdempotencyCleanupJob for hourly expired key cleanup
- application.yml additions for TTL and cleanup config
- 5 unit tests (IdempotencyKeyFilterTest)
- 4 integration tests (IdempotencyKeyFilterIT)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAdds JDBC-backed request idempotency across multiple services: new IdempotencyKeyFilter (reservation, hash, replay, PUT support), scheduled IdempotencyCleanupJob, DB migrations for idempotency tables (and scope-change to composite PK), integration/unit tests, config entries, and testcontainer JVM shutdown hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Filter as IdempotencyKeyFilter
participant DB as JdbcTemplate
participant Chain as FilterChain
participant Handler as DownstreamHandler
Client->>Filter: Mutating request (Idempotency-Key + body)
Filter->>Filter: compute SHA-256(requestBody)
Filter->>DB: SELECT lookup by idempotency_key[, method, path]
alt record exists
DB-->>Filter: IdempotencyRecord(status, request_hash, response_body, status_code)
Filter->>Filter: compare request_hash
alt hashes match and status finalized
Filter->>Client: return cached response + Idempotency-Replay header
else hash mismatch
Filter->>Client: 422 Unprocessable Entity
else in-flight
Filter->>Client: 409 Conflict
end
else not found
Filter->>DB: INSERT reservation (status=in-flight, expires_at)
Filter->>Chain: doFilter(wrapped request)
Chain->>Handler: process
Handler-->>Chain: response
Chain-->>Filter: response
Filter->>DB: UPDATE record with response_body, status_code, finalize
Filter->>Client: return response
end
sequenceDiagram
participant Scheduler as Spring Scheduler
participant Job as IdempotencyCleanupJob
participant DB as JdbcTemplate
participant Log as Logger
Scheduler->>Job: cron trigger
activate Job
loop batch deletes
Job->>DB: DELETE expired rows (CTE) LIMIT 1000
DB-->>Job: deleted_count
end
alt deleted_count > 0
Job->>Log: log total deleted
end
deactivate Job
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 14
♻️ Duplicate comments (1)
merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java (1)
140-159:⚠️ Potential issue | 🟠 MajorSame issue: persisting idempotency records for error responses.
This filter has the same behavior as merchant-onboarding — persisting records regardless of response status. Failed requests get locked in and replayed on retry.
Apply the same fix to only persist 2xx responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java` around lines 140 - 159, The persistIdempotencyKey method is persisting all responses including failures; modify persistIdempotencyKey (and its call-sites if needed) to only persist when the wrappedResponse status is a successful 2xx (check statusCode with something like statusCode >= 200 && statusCode < 300) and return/skip the JDBC insert for non-2xx responses; keep existing concurrency handling (DataIntegrityViolationException and lookupIdempotencyKey) unchanged but ensure no DB write occurs for error responses so failed requests aren't locked in by TABLE_NAME inserts and ttlHours logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.java`:
- Around line 32-141: Add a new concurrent integration test in
IdempotencyKeyFilterIT (e.g., shouldHandleConcurrentRequests_sameKeyConcurrent)
that submits two POSTs in parallel with the same "Idempotency-Key" and identical
body using MockMvc + an ExecutorService or CompletableFutures and a
CountDownLatch to synchronize start; from the two responses assert one resulted
in creation and the other either contains header "Idempotency-Replay" = "true"
(or returns the same response body) and then verify via jdbcTemplate that only
one row exists for that idempotency key (query SELECT COUNT(*) FROM
gatewayiam_idempotency_keys WHERE idempotency_key = ? and assert count == 1);
ensure the test uses the existing MERCHANT_ENDPOINT, builds the same requestBody
and same idempotencyKey, and cleans up any inserted test data if needed.
In
`@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 142-146: Remove logging of raw idempotency keys in
IdempotencyKeyFilter: do not call log.debug or log.warn with the plain key
value. Instead, log a safe identifier (e.g. a truncated/masked form or a stable
hash) so the key itself is never emitted; update the debug line that currently
logs "Concurrent idempotency key insertion for key={}" and the warn line that
logs "Concurrent idempotency key conflict: key={}" to use the masked/hashed
value, referencing the existing lookupIdempotencyKey(...) call and the
requestHash comparison to preserve context without exposing the raw token.
- Around line 88-103: The current flow calls chain.doFilter (executing the
request) before persistIdempotencyKey, allowing a TOCTOU race where two
concurrent requests with the same key both mutate state; change the flow to
atomically reserve the idempotency key before executing the request by adding a
reserveIdempotencyKey(key, requestHash) step that performs an atomic insert/lock
(or upsert with "insert if not exists"), check lookupIdempotencyKey after
reserve to handle existing entries (call replayResponse or
writeHashMismatchError as appropriate), then proceed to wrap the request
(CachedBodyRequestWrapper) and response (ContentCachingResponseWrapper), call
chain.doFilter, and finally call a finalizePersistIdempotencyKey(key,
requestHash, wrappedResponse) to update the reserved record with the response;
apply the same reserve-first then finalize pattern to the other similar block
around persistIdempotencyKey (lines 129-147).
- Around line 117-120: The SELECT in IdempotencyKeyFilter uses
jdbcTemplate.query against TABLE_NAME but does not filter out expired keys,
allowing stale entries to be returned; update the SQL to include a TTL check
(e.g. "AND (expires_at IS NULL OR expires_at > ?)" or "AND created_at + INTERVAL
'1 second' * ttl > ?" depending on schema) and pass Instant.now()/current
timestamp as an extra query parameter so IdempotencyRecord is only built from
non-expired rows; adjust the parameter list and keep the mapping to new
IdempotencyRecord unchanged.
In
`@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/job/IdempotencyCleanupJob.java`:
- Around line 20-21: The current unbounded delete in IdempotencyCleanupJob
(jdbcTemplate.update("DELETE FROM gatewayiam_idempotency_keys WHERE expires_at <
NOW()")) can cause table-level pressure; change it to perform batched deletes
instead by looping and issuing a DELETE limited to a safe batch size (use the
table primary key or LIMIT semantics) until fewer than batchSize rows are
removed, committing between batches and optionally adding a short backoff;
implement this logic inside the IdempotencyCleanupJob run method (or the method
calling jdbcTemplate.update) using the existing jdbcTemplate to execute the
repeated limited deletes and stop when no more expired rows remain.
In
`@api-gateway-iam/api-gateway-iam/src/main/resources/db/migration/V8__create_idempotency_keys_table.sql`:
- Around line 1-3: The idempotency key must be scoped to prevent
cross-principal/endpoint collision: modify the migration to add an
idempotency_scope column (e.g., VARCHAR(255) NOT NULL) and make the primary key
composite (idempotency_key, idempotency_scope) instead of key alone; then update
the DB access in IdempotencyKeyFilter.java to include idempotency_scope in both
SELECT and INSERT statements and in any lookups or deletes so replay isolation
is enforced by scope. Ensure you update any prepared statement parameters and
result mapping in IdempotencyKeyFilter.java to pass and read the scope alongside
the key.
In
`@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java`:
- Around line 153-168: Add a new unit test in IdempotencyKeyFilterTest that
mirrors the POST test but uses a MockHttpServletRequest with method "PUT" to
assert mutating-method idempotency enforcement: create request = new
MockHttpServletRequest("PUT", "/v1/merchants"), call
filter.doFilterInternal(request, response, filterChain), then assert the
expected behavior (e.g., status and interactions) the same way other mutating
tests do—verify filterChain.doFilter was called or blocked as appropriate and
that jdbcTemplate interactions occur/are skipped consistent with POST; use the
same helper objects (filter, response, filterChain, jdbcTemplate) and naming
patterns as existing tests for consistency.
- Around line 131-150: The test should assert the persistence side-effect: after
calling IdempotencyKeyFilterTest.shouldProceedAndPersist_whenKeyNotFound and
filter.doFilterInternal(request, response, filterChain), verify that
jdbcTemplate.update(...) was invoked to insert the new idempotency record; add a
Mockito then(jdbcTemplate).should().update(...) (or equivalent verify) assertion
that matches the INSERT SQL used by IdempotencyKeyFilter (or
startsWith("INSERT")) and the expected parameters (e.g., the idempotency key
"key-789" and any other values the filter inserts) so the test fails if the
insert path regresses.
In
`@merchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterIT.java`:
- Around line 33-55: The `@BeforeEach` method seedBuiltInRoles does not clear the
merchantiam_idempotency_keys table, causing flaky tests; modify seedBuiltInRoles
to delete existing idempotency keys before seeding (either by invoking the
existing cleanIdempotencyKeys() helper if available or executing a DELETE on
merchantiam_idempotency_keys via jdbcTemplate) so each test starts with a clean
idempotency state; update references in the test class (seedBuiltInRoles,
cleanIdempotencyKeys, jdbcTemplate) accordingly.
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 179-249: Extract the duplicated idempotency pieces into a new
shared module (e.g. stablebridge-idempotency) and replace the in-repo copies
with references to that library: move computeSha256, the IdempotencyRecord
record, the CachedBodyRequestWrapper class and the core logic currently in
IdempotencyKeyFilter into the shared module as reusable components; make the
shared module configurable for table name, error code prefix and exempt paths
(injectable via constructor or Spring properties) and expose a pluggable filter
or service API that the existing IdempotencyKeyFilter can instantiate or
delegate to; update api-gateway-iam, merchant-iam and merchant-onboarding to
depend on the new module and remove the duplicated symbols
(CachedBodyRequestWrapper, computeSha256, IdempotencyRecord, and the
IdempotencyKeyFilter logic) so each project supplies its configuration only.
In
`@merchant-iam/merchant-iam/src/main/resources/db/migration/V10__create_idempotency_keys_table.sql`:
- Around line 1-8: The current primary key on merchantiam_idempotency_keys is
too broad; alter the table definition to make the PK composite so
idempotency_key is scoped to request context (e.g., add columns request_method
VARCHAR(...), request_path VARCHAR(...), and principal_id or tenant_id
VARCHAR(...) and use (idempotency_key, request_method, request_path,
principal_id) as the PRIMARY KEY), ensure these new columns are NOT NULL (or
nullable per your auth model) and adjust any lookup/INSERT/UPSERT predicates and
indexes that reference merchantiam_idempotency_keys (including queries that use
idempotency_key alone) to include the same scoping columns so lookups/insertions
use the full composite key for uniqueness and replay protection.
In
`@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.java`:
- Around line 107-124: The test directly constructs IdempotencyCleanupJob,
bypassing Spring's `@ConditionalOnProperty`; change the test in
IdempotencyKeyFilterIT to obtain the bean from the Spring context instead:
inject ApplicationContext (or Autowire Optional<IdempotencyCleanupJob> /
`@Autowired`(required = false) IdempotencyCleanupJob) and assert the bean is
present when the property is enabled, then call cleanupJob.cleanExpiredKeys();
alternatively add a separate small test that boots the context with the property
set and verifies that application bean IdempotencyCleanupJob is created (or
absent when property is false) to cover the conditional wiring.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/job/IdempotencyCleanupJob.java`:
- Around line 20-21: The current single-statement delete in
IdempotencyCleanupJob (the jdbcTemplate.update call assigning to deleted)
performs an unbounded DELETE which can cause long transactions and locks; change
the implementation to perform batched deletes: execute a loop that issues
repeated jdbcTemplate.update calls with a chunked DELETE (e.g., "DELETE FROM
onboarding_idempotency_keys WHERE expires_at < NOW() LIMIT :chunkSize") until
the update returns 0, run each batch in its own short transaction, and
optionally include a small backoff/sleep between iterations; ensure you
reference the existing jdbcTemplate.update usage and replace the single
unbounded delete with this controlled batching pattern.
In
`@merchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V16__create_idempotency_keys_table.sql`:
- Around line 1-3: The idempotency key is currently globally unique
(onboarding_idempotency_keys.idempotency_key) which allows cross-context replay;
modify the schema to include explicit scope columns (e.g., actor_id, route,
method or a single request_scope) and make the uniqueness/primary key composite
with idempotency_key plus those scope columns (rather than idempotency_key
alone), keep request_hash as-is, and then update all lookup/insert logic that
queries or writes onboarding_idempotency_keys to include the same scope columns
in WHERE/INSERT clauses so lookups are scoped to the same actor/route/method
used when the key was created.
---
Duplicate comments:
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 140-159: The persistIdempotencyKey method is persisting all
responses including failures; modify persistIdempotencyKey (and its call-sites
if needed) to only persist when the wrappedResponse status is a successful 2xx
(check statusCode with something like statusCode >= 200 && statusCode < 300) and
return/skip the JDBC insert for non-2xx responses; keep existing concurrency
handling (DataIntegrityViolationException and lookupIdempotencyKey) unchanged
but ensure no DB write occurs for error responses so failed requests aren't
locked in by TABLE_NAME inserts and ttlHours logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d4d55b9-9c38-42ec-9c1c-d5d8fa58607f
📒 Files selected for processing (23)
api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.javaapi-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/job/IdempotencyCleanupJob.javaapi-gateway-iam/api-gateway-iam/src/main/resources/application-integration-test.ymlapi-gateway-iam/api-gateway-iam/src/main/resources/application.ymlapi-gateway-iam/api-gateway-iam/src/main/resources/db/migration/V8__create_idempotency_keys_table.sqlapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterIT.javamerchant-iam/merchant-iam/src/integration-test/resources/application-integration-test.ymlmerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/job/IdempotencyCleanupJob.javamerchant-iam/merchant-iam/src/main/resources/application.ymlmerchant-iam/merchant-iam/src/main/resources/db/migration/V10__create_idempotency_keys_table.sqlmerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.javamerchant-onboarding/merchant-onboarding/src/integration-test/resources/application-integration-test.ymlmerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/job/IdempotencyCleanupJob.javamerchant-onboarding/merchant-onboarding/src/main/resources/application.ymlmerchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V16__create_idempotency_keys_table.sqlmerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java
Comment on lines
+32
to
+141
| @Test | ||
| @DisplayName("should persist idempotency key after successful mutation") | ||
| void shouldPersistIdempotencyKey_afterSuccessfulMutation() throws Exception { | ||
| var idempotencyKey = UUID.randomUUID().toString(); | ||
| var externalId = UUID.randomUUID(); | ||
|
|
||
| mockMvc.perform(post(MERCHANT_ENDPOINT) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .content(""" | ||
| { | ||
| "externalId": "%s", | ||
| "name": "Idempotency Test Corp", | ||
| "country": "US" | ||
| } | ||
| """.formatted(externalId))) | ||
| .andExpect(status().isCreated()); | ||
|
|
||
| var count = jdbcTemplate.queryForObject( | ||
| "SELECT COUNT(*) FROM gatewayiam_idempotency_keys WHERE idempotency_key = ?", | ||
| Integer.class, idempotencyKey); | ||
|
|
||
| assertThat(count).isEqualTo(1); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should replay response on duplicate request with same key and body") | ||
| void shouldReplayResponse_onDuplicateRequest() throws Exception { | ||
| var idempotencyKey = UUID.randomUUID().toString(); | ||
| var externalId = UUID.randomUUID(); | ||
| var requestBody = """ | ||
| { | ||
| "externalId": "%s", | ||
| "name": "Replay Test Corp", | ||
| "country": "US" | ||
| } | ||
| """.formatted(externalId); | ||
|
|
||
| // First request | ||
| var firstResponse = mockMvc.perform(post(MERCHANT_ENDPOINT) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .content(requestBody)) | ||
| .andExpect(status().isCreated()) | ||
| .andReturn(); | ||
|
|
||
| // Second request — same key, same body | ||
| var secondResponse = mockMvc.perform(post(MERCHANT_ENDPOINT) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .content(requestBody)) | ||
| .andExpect(status().isCreated()) | ||
| .andExpect(header().string("Idempotency-Replay", "true")) | ||
| .andReturn(); | ||
|
|
||
| assertThat(secondResponse.getResponse().getContentAsString()) | ||
| .isEqualTo(firstResponse.getResponse().getContentAsString()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return 422 when same key but different body") | ||
| void shouldReturn422_whenSameKeyDifferentBody() throws Exception { | ||
| var idempotencyKey = UUID.randomUUID().toString(); | ||
|
|
||
| // First request | ||
| mockMvc.perform(post(MERCHANT_ENDPOINT) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .content(""" | ||
| { | ||
| "externalId": "%s", | ||
| "name": "First Corp", | ||
| "country": "US" | ||
| } | ||
| """.formatted(UUID.randomUUID()))) | ||
| .andExpect(status().isCreated()); | ||
|
|
||
| // Second request — same key, different body | ||
| mockMvc.perform(post(MERCHANT_ENDPOINT) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .header("Idempotency-Key", idempotencyKey) | ||
| .content(""" | ||
| { | ||
| "externalId": "%s", | ||
| "name": "Different Corp", | ||
| "country": "GB" | ||
| } | ||
| """.formatted(UUID.randomUUID()))) | ||
| .andExpect(status().isUnprocessableEntity()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should delete expired keys when cleanup job runs") | ||
| void shouldDeleteExpiredKeys_whenCleanupJobRuns() throws Exception { | ||
| // Insert an expired idempotency key directly | ||
| jdbcTemplate.update( | ||
| "INSERT INTO gatewayiam_idempotency_keys (idempotency_key, request_hash, response_body, status_code, expires_at) " | ||
| + "VALUES (?, ?, ?, ?, ?)", | ||
| "expired-key", "somehash", "{}", 200, | ||
| Timestamp.from(Instant.now().minus(1, ChronoUnit.HOURS))); | ||
|
|
||
| var cleanupJob = new com.stablecoin.payments.gateway.iam.application.job.IdempotencyCleanupJob(jdbcTemplate); | ||
| cleanupJob.cleanExpiredKeys(); | ||
|
|
||
| var count = jdbcTemplate.queryForObject( | ||
| "SELECT COUNT(*) FROM gatewayiam_idempotency_keys WHERE idempotency_key = ?", | ||
| Integer.class, "expired-key"); | ||
|
|
||
| assertThat(count).isEqualTo(0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a concurrent duplicate-request integration test.
Current suite does not verify the “same key submitted concurrently” path. Add a parallel POST test asserting a single mutation and one replay/duplicate handling path to lock down the core guarantee.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.java`
around lines 32 - 141, Add a new concurrent integration test in
IdempotencyKeyFilterIT (e.g., shouldHandleConcurrentRequests_sameKeyConcurrent)
that submits two POSTs in parallel with the same "Idempotency-Key" and identical
body using MockMvc + an ExecutorService or CompletableFutures and a
CountDownLatch to synchronize start; from the two responses assert one resulted
in creation and the other either contains header "Idempotency-Replay" = "true"
(or returns the same response body) and then verify via jdbcTemplate that only
one row exists for that idempotency key (query SELECT COUNT(*) FROM
gatewayiam_idempotency_keys WHERE idempotency_key = ? and assert count == 1);
ensure the test uses the existing MERCHANT_ENDPOINT, builds the same requestBody
and same idempotencyKey, and cleans up any inserted test data if needed.
Comment on lines
+179
to
+249
| private String computeSha256(byte[] body) { | ||
| try { | ||
| var digest = MessageDigest.getInstance("SHA-256"); | ||
| var hash = digest.digest(body); | ||
| return HexFormat.of().formatHex(hash); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IllegalStateException("SHA-256 not available", e); | ||
| } | ||
| } | ||
|
|
||
| record IdempotencyRecord(String idempotencyKey, String requestHash, String responseBody, int statusCode) {} | ||
|
|
||
| /** | ||
| * Request wrapper that replays a cached body so downstream filters and | ||
| * controllers can read the request body after it has already been consumed. | ||
| */ | ||
| private static class CachedBodyRequestWrapper extends HttpServletRequestWrapper { | ||
|
|
||
| private final byte[] body; | ||
|
|
||
| CachedBodyRequestWrapper(HttpServletRequest request, byte[] body) { | ||
| super(request); | ||
| this.body = body; | ||
| } | ||
|
|
||
| @Override | ||
| public ServletInputStream getInputStream() { | ||
| var byteStream = new ByteArrayInputStream(body); | ||
| return new ServletInputStream() { | ||
| @Override | ||
| public boolean isFinished() { | ||
| return byteStream.available() == 0; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isReady() { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public void setReadListener(ReadListener readListener) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public int read() { | ||
| return byteStream.read(); | ||
| } | ||
|
|
||
| @Override | ||
| public int read(byte[] b, int off, int len) { | ||
| return byteStream.read(b, off, len); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public BufferedReader getReader() { | ||
| return new BufferedReader(new InputStreamReader(getInputStream(), StandardCharsets.UTF_8)); | ||
| } | ||
|
|
||
| @Override | ||
| public int getContentLength() { | ||
| return body.length; | ||
| } | ||
|
|
||
| @Override | ||
| public long getContentLengthLong() { | ||
| return body.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared idempotency infrastructure.
CachedBodyRequestWrapper, computeSha256, IdempotencyRecord, and core filter logic are duplicated across api-gateway-iam, merchant-iam, and merchant-onboarding.
Extract to a shared stablebridge-idempotency module with configurable table name, error code prefix, and exempt paths to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java`
around lines 179 - 249, Extract the duplicated idempotency pieces into a new
shared module (e.g. stablebridge-idempotency) and replace the in-repo copies
with references to that library: move computeSha256, the IdempotencyRecord
record, the CachedBodyRequestWrapper class and the core logic currently in
IdempotencyKeyFilter into the shared module as reusable components; make the
shared module configurable for table name, error code prefix and exempt paths
(injectable via constructor or Spring properties) and expose a pluggable filter
or service API that the existing IdempotencyKeyFilter can instantiate or
delegate to; update api-gateway-iam, merchant-iam and merchant-onboarding to
depend on the new module and remove the duplicated symbols
(CachedBodyRequestWrapper, computeSha256, IdempotencyRecord, and the
IdempotencyKeyFilter logic) so each project supplies its configuration only.
Comment on lines
+1
to
+8
| CREATE TABLE merchantiam_idempotency_keys ( | ||
| idempotency_key VARCHAR(255) PRIMARY KEY, | ||
| request_hash VARCHAR(64) NOT NULL, | ||
| response_body TEXT NOT NULL DEFAULT '', | ||
| status_code INTEGER NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| expires_at TIMESTAMPTZ NOT NULL | ||
| ); |
There was a problem hiding this comment.
Primary key scope is too broad for secure idempotency replay.
Using only idempotency_key as PK can mix requests across endpoints/actors that reuse the same key. Scope uniqueness to request context (at least method + path, and ideally principal/tenant) to prevent wrong-response replay.
Proposed schema direction
CREATE TABLE merchantiam_idempotency_keys (
- idempotency_key VARCHAR(255) PRIMARY KEY,
+ idempotency_key VARCHAR(255) NOT NULL,
+ request_method VARCHAR(10) NOT NULL,
+ request_path VARCHAR(255) NOT NULL,
request_hash VARCHAR(64) NOT NULL,
response_body TEXT NOT NULL DEFAULT '',
status_code INTEGER NOT NULL,
created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
- expires_at TIMESTAMPTZ NOT NULL
+ expires_at TIMESTAMPTZ NOT NULL,
+ CONSTRAINT pk_merchantiam_idempotency
+ PRIMARY KEY (request_method, request_path, idempotency_key)
);Also update filter lookup/insert predicates to include the same scoping columns.
📝 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.
Suggested change
| CREATE TABLE merchantiam_idempotency_keys ( | |
| idempotency_key VARCHAR(255) PRIMARY KEY, | |
| request_hash VARCHAR(64) NOT NULL, | |
| response_body TEXT NOT NULL DEFAULT '', | |
| status_code INTEGER NOT NULL, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| expires_at TIMESTAMPTZ NOT NULL | |
| ); | |
| CREATE TABLE merchantiam_idempotency_keys ( | |
| idempotency_key VARCHAR(255) NOT NULL, | |
| request_method VARCHAR(10) NOT NULL, | |
| request_path VARCHAR(255) NOT NULL, | |
| request_hash VARCHAR(64) NOT NULL, | |
| response_body TEXT NOT NULL DEFAULT '', | |
| status_code INTEGER NOT NULL, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| expires_at TIMESTAMPTZ NOT NULL, | |
| CONSTRAINT pk_merchantiam_idempotency | |
| PRIMARY KEY (request_method, request_path, idempotency_key) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-iam/merchant-iam/src/main/resources/db/migration/V10__create_idempotency_keys_table.sql`
around lines 1 - 8, The current primary key on merchantiam_idempotency_keys is
too broad; alter the table definition to make the PK composite so
idempotency_key is scoped to request context (e.g., add columns request_method
VARCHAR(...), request_path VARCHAR(...), and principal_id or tenant_id
VARCHAR(...) and use (idempotency_key, request_method, request_path,
principal_id) as the PRIMARY KEY), ensure these new columns are NOT NULL (or
nullable per your auth model) and adjust any lookup/INSERT/UPSERT predicates and
indexes that reference merchantiam_idempotency_keys (including queries that use
idempotency_key alone) to include the same scoping columns so lookups/insertions
use the full composite key for uniqueness and replay protection.
Comment on lines
+107
to
+124
| @Test | ||
| @DisplayName("should delete expired keys when cleanup job runs") | ||
| void shouldDeleteExpiredKeys_whenCleanupJobRuns() throws Exception { | ||
| jdbcTemplate.update( | ||
| "INSERT INTO onboarding_idempotency_keys (idempotency_key, request_hash, response_body, status_code, expires_at) " | ||
| + "VALUES (?, ?, ?, ?, ?)", | ||
| "expired-key", "somehash", "{}", 200, | ||
| Timestamp.from(Instant.now().minus(1, ChronoUnit.HOURS))); | ||
|
|
||
| var cleanupJob = new com.stablecoin.payments.merchant.onboarding.application.job.IdempotencyCleanupJob(jdbcTemplate); | ||
| cleanupJob.cleanExpiredKeys(); | ||
|
|
||
| var count = jdbcTemplate.queryForObject( | ||
| "SELECT COUNT(*) FROM onboarding_idempotency_keys WHERE idempotency_key = ?", | ||
| Integer.class, "expired-key"); | ||
|
|
||
| assertThat(count).isEqualTo(0); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cleanup test bypasses Spring's conditional bean creation.
Direct instantiation of IdempotencyCleanupJob bypasses the @ConditionalOnProperty check. This tests the cleanup logic but not the Spring wiring.
Consider injecting the bean when available, or add a separate config verification test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.java`
around lines 107 - 124, The test directly constructs IdempotencyCleanupJob,
bypassing Spring's `@ConditionalOnProperty`; change the test in
IdempotencyKeyFilterIT to obtain the bean from the Spring context instead:
inject ApplicationContext (or Autowire Optional<IdempotencyCleanupJob> /
`@Autowired`(required = false) IdempotencyCleanupJob) and assert the bean is
present when the property is enabled, then call cleanupJob.cleanExpiredKeys();
alternatively add a separate small test that boots the context with the property
set and verifies that application bean IdempotencyCleanupJob is created (or
absent when property is false) to cover the conditional wiring.
Comment on lines
+1
to
+3
| CREATE TABLE onboarding_idempotency_keys ( | ||
| idempotency_key VARCHAR(255) PRIMARY KEY, | ||
| request_hash VARCHAR(64) NOT NULL, |
There was a problem hiding this comment.
Critical: schema does not isolate idempotency keys by request scope.
Line 2 makes idempotency_key globally unique; without scope (actor + route/method), replay boundaries can cross request contexts and return the wrong cached response.
🔧 Proposed schema hardening
CREATE TABLE onboarding_idempotency_keys (
- idempotency_key VARCHAR(255) PRIMARY KEY,
+ idempotency_key VARCHAR(255) NOT NULL,
+ idempotency_scope VARCHAR(255) NOT NULL,
request_hash VARCHAR(64) NOT NULL,
response_body TEXT NOT NULL DEFAULT '',
status_code INTEGER NOT NULL,
created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
- expires_at TIMESTAMPTZ NOT NULL
+ expires_at TIMESTAMPTZ NOT NULL,
+ PRIMARY KEY (idempotency_key, idempotency_scope)
);Filter lookup/insert logic must include the same scope key.
📝 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.
Suggested change
| CREATE TABLE onboarding_idempotency_keys ( | |
| idempotency_key VARCHAR(255) PRIMARY KEY, | |
| request_hash VARCHAR(64) NOT NULL, | |
| CREATE TABLE onboarding_idempotency_keys ( | |
| idempotency_key VARCHAR(255) NOT NULL, | |
| idempotency_scope VARCHAR(255) NOT NULL, | |
| request_hash VARCHAR(64) NOT NULL, | |
| response_body TEXT NOT NULL DEFAULT '', | |
| status_code INTEGER NOT NULL, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| expires_at TIMESTAMPTZ NOT NULL, | |
| PRIMARY KEY (idempotency_key, idempotency_scope) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V16__create_idempotency_keys_table.sql`
around lines 1 - 3, The idempotency key is currently globally unique
(onboarding_idempotency_keys.idempotency_key) which allows cross-context replay;
modify the schema to include explicit scope columns (e.g., actor_id, route,
method or a single request_scope) and make the uniqueness/primary key composite
with idempotency_key plus those scope columns (rather than idempotency_key
alone), keep request_hash as-is, and then update all lookup/insert logic that
queries or writes onboarding_idempotency_keys to include the same scope columns
in WHERE/INSERT clauses so lookups are scoped to the same actor/route/method
used when the key was created.
…leanup (STA-72) - Add request_method + request_path to composite PK for endpoint-scoped idempotency - INSERT-first reservation pattern to prevent TOCTOU race on concurrent duplicates - Filter expired keys at read time (AND expires_at > NOW()) - Batched DELETE in cleanup jobs to avoid lock spikes - Remove raw idempotency keys from log messages - Add missing table cleanup in S13 IT @beforeeach - Add 409 Conflict response for in-flight duplicate requests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (3)
merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.java (1)
107-118:⚠️ Potential issue | 🟡 MinorUse the Spring-managed cleanup bean in this integration test.
The direct
new IdempotencyCleanupJob(...)on Line 117 only exercises the method body; it does not verify the@ConditionalOnPropertywiring for the scheduled bean. Autowire the bean from the test context and invoke that instance here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.java` around lines 107 - 118, The test currently constructs a new IdempotencyCleanupJob instance directly (new com.stablecoin.payments.merchant.onboarding.application.job.IdempotencyCleanupJob(jdbcTemplate)), which bypasses Spring wiring and the `@ConditionalOnProperty` behavior; instead, autowire the Spring-managed bean of type com.stablecoin.payments.merchant.onboarding.application.job.IdempotencyCleanupJob into the test (e.g., add an `@Autowired` field idempotencyCleanupJob) and call idempotencyCleanupJob.cleanExpiredKeys() so the test exercises the actual scheduled/conditional bean from the application context.api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java (1)
114-125:⚠️ Potential issue | 🟠 MajorAssert the reservation-path side effects.
A 200 alone does not prove that the request reached the filter chain or that the reserved row was finalized. Verify
filterChain.doFilter(...)and theUPDATE/finalize interaction so this test fails if the happy path regresses.As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".Suggested assertions
filter.doFilterInternal(request, response, filterChain); assertThat(response.getStatus()).isEqualTo(200); + then(filterChain).should().doFilter( + org.mockito.ArgumentMatchers.any(), + org.mockito.ArgumentMatchers.any()); + then(jdbcTemplate).should().update( + org.mockito.ArgumentMatchers.startsWith("UPDATE"), + org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.eq(200), + org.mockito.ArgumentMatchers.any(), + org.mockito.ArgumentMatchers.eq("key-789"), + org.mockito.ArgumentMatchers.eq("POST"), + org.mockito.ArgumentMatchers.eq("/v1/merchants"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java` around lines 114 - 125, The test should assert side effects beyond the 200 status: after calling filter.doFilterInternal in shouldProceedAndFinalize_whenReservationSucceeds, verify that filterChain.doFilter(request, response) was invoked to ensure the request reached the chain, and verify that the idempotency reservation was finalized by asserting a call to the finalize/update method (e.g., finalizeIdempotencyKey or finalizeReservation) with the expected key "key-789" and request metadata; keep the existing use of reserveIdempotencyKey("key-789", "POST", "/v1/merchants", REQUEST_HASH) and add verify(filterChain).doFilter(...) and verify(...) for the finalization call so the test fails if the happy-path finalization regresses.api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.java (1)
32-142:⚠️ Potential issue | 🟠 MajorAdd a concurrent duplicate-request integration test.
This suite proves sequential replay, but the highest-risk path in this PR is two same-key POSTs racing on the reservation INSERT. Add a parallel POST test that asserts one create plus one replay/409 outcome and exactly one persisted row for the composite key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.java` around lines 32 - 142, Add a new concurrent integration test to IdempotencyKeyFilterIT that races two POSTs with the same Idempotency-Key to MERCHANT_ENDPOINT: create a test method (e.g., shouldHandleConcurrentDuplicateRequests) that uses ExecutorService (or CompletableFuture) and a CountDownLatch to start two mockMvc.post calls simultaneously with the same idempotencyKey and identical requestBody, collect both MockHttpServletResponses, and assert that exactly one request resulted in a successful create (status 201) while the other resulted in either a replay (status 201 with header "Idempotency-Replay" == "true") or a conflict (status 409); finally use jdbcTemplate.queryForObject to assert the gatewayiam_idempotency_keys row count for that idempotencyKey (and same request_hash/composite key) equals 1. Ensure you reference MERCHANT_ENDPOINT, mockMvc, jdbcTemplate, and the idempotency key/request body variables when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 159-178: The replay currently hardcodes MediaType.APPLICATION_JSON
and only stores response_body and status_code; update finalizeIdempotencyKey to
also persist the response content type and any replay-relevant headers (e.g.,
Content-Type, Location, Cache-Control) into the idempotency record (extend
TABLE_NAME schema and the JDBC UPDATE to include content_type and headers),
update the IdempotencyRecord to hold contentType and a map/string of headers,
and modify replayResponse to restore
response.setContentType(record.contentType()) and iterate/restore the stored
headers (and remove the hard-coded application/json) before writing
record.responseBody(); ensure the JDBC read path that builds IdempotencyRecord
populates these new fields.
- Around line 93-100: The current reservation made by reserveIdempotencyKey(...)
is only finalized in the success path, so exceptions leave the row stuck at
status_code=0; wrap the processing block (creation of CachedBodyRequestWrapper,
ContentCachingResponseWrapper and chain.doFilter(...)) in a try/catch (or
try/catch/finally) and ensure finalizeIdempotencyKey(key, method, path,
wrappedResponse) is invoked on success and a dedicated failure-finalizer (or a
call that records a terminal failure/clears the reservation) is invoked in the
catch before rethrowing the exception; reference reserveIdempotencyKey,
CachedBodyRequestWrapper, ContentCachingResponseWrapper, chain.doFilter, and
finalizeIdempotencyKey to locate where to add the error-path finalization so no
reservation remains at status_code=0 after an exception.
- Around line 130-156: reserveIdempotencyKey currently returns false on
DataIntegrityViolationException without attempting to reclaim expired rows;
change the catch path to try to overwrite/reclaim a stale record by issuing an
UPDATE that sets request_hash, response_body='', status_code=0 and extends
expires_at (WHERE idempotency_key = ?, request_method = ?, request_path = ? AND
expires_at <= NOW()), and if that update affects >0 rows return true, otherwise
return false; reference reserveIdempotencyKey for the INSERT and the
lookupIdempotencyKey behavior so the reclaim UPDATE matches the same composite
key and TTL semantics.
In
`@api-gateway-iam/api-gateway-iam/src/main/resources/db/migration/V9__scope_idempotency_keys.sql`:
- Line 1: Add a short SQL comment immediately above the ALTER TABLE statement
explaining why the primary key is being dropped and why the operation is safe
for existing rows; reference the gatewayiam_idempotency_keys table and the
gatewayiam_idempotency_keys_pkey constraint in the comment and state the
justification (e.g., replacing PK with new constraint, fixing schema mismatch,
or non-destructive migration validated against existing data) so reviewers can
understand the destructive change before the ALTER TABLE DROP CONSTRAINT
gatewayiam_idempotency_keys_pkey line.
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 249-252: The override of setReadListener in IdempotencyKeyFilter
currently throws UnsupportedOperationException which breaks async servlet
requests; change the implementation to delegate to the wrapped
ServletInputStream's setReadListener instead of throwing (e.g., call
originalInputStream.setReadListener(readListener)), ensuring async support is
preserved and behavior matches other modules that forward async callbacks.
- Around line 100-109: The reservation returned by reserveIdempotencyKey can
remain in-flight (status_code=0) if chain.doFilter throws and
finalizeIdempotencyKey is never called; add an explicit comment next to the
reserveIdempotencyKey/chain.doFilter block (referencing reserveIdempotencyKey,
finalizeIdempotencyKey, CachedBodyRequestWrapper, ContentCachingResponseWrapper)
explaining that this behavior is intentional: failed filter execution leaves the
reservation open to block retries (409 Conflict) until TTL expiry, and note that
callers may change this to release on error if desired for different semantics.
- Around line 97-98: The filter currently calls
request.getInputStream().readAllBytes() (seen near computeSha256 in
IdempotencyKeyFilter) without bounding the body size; add a guard to prevent DoS
by either configuring Tomcat max-http-post-size in application.yml (e.g. 10MB)
or, immediately before reading the stream in IdempotencyKeyFilter, check the
Content-Length/request.getContentLength() against a MAX_BODY_SIZE constant and
if exceeded set HttpStatus.PAYLOAD_TOO_LARGE on the response and return; ensure
MAX_BODY_SIZE is defined and used consistently so computeSha256 is only invoked
on safely sized payloads.
In
`@merchant-iam/merchant-iam/src/main/resources/db/migration/V11__scope_idempotency_keys.sql`:
- Line 1: Add an inline SQL comment immediately before the ALTER TABLE statement
that drops the primary key on merchantiam_idempotency_keys (ALTER TABLE
merchantiam_idempotency_keys DROP CONSTRAINT merchantiam_idempotency_keys_pkey),
briefly stating the justification and rollback context (e.g., why the PK is
being replaced/modified and how to revert) to satisfy the destructive-operations
guideline and aid audits.
- Around line 2-4: The migration adds request_method and request_path and a new
PK (idempotency_key, request_method, request_path) which backfills existing rows
into a scope that won't match real endpoint reads and will break retry semantics
for active keys; add a pre-migration guard in the same SQL migration that checks
merchantiam_idempotency_keys for any active/unexpired rows (e.g., WHERE
expires_at > now()) and RAISE EXCEPTION (or abort) with a clear message if any
exist so the migration fails rather than silently changing scope for live keys,
or alternatively abort and instruct operators to expire or migrate active rows
before proceeding; reference the merchantiam_idempotency_keys table and the new
columns request_method and request_path and the new PK tuple when implementing
this guard.
In
`@merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-126: The test shouldProceedAndFinalize_whenReservationSucceeds
in IdempotencyKeyFilterTest currently asserts response.getStatus() which is the
MockHttpServletResponse default; change the assertion to verify the filter
actually continued the chain by using Mockito to verify that
filterChain.doFilter(request, response) was invoked (e.g.,
verify(filterChain).doFilter(any(ServletRequest.class),
any(ServletResponse.class)) or the exact request/response), and remove or
replace the status assertion so the test validates that
IdempotencyKeyFilter.doFilterInternal allowed the chain to proceed.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.java`:
- Around line 57-64: The IdempotencyKeyFilter is coupled to JDBC via
JdbcTemplate and ttlHours; replace those concrete dependencies with a domain
port (e.g., an IdempotencyStore/IdempotencyRepository interface) injected into
IdempotencyKeyFilter and move the current SQL/JDBC implementation into the
infrastructure layer; update the constructor of IdempotencyKeyFilter to accept
the IdempotencyStore (and any config like ttlHours via configuration properties
or a domain-specific config object) and delegate all DB operations to the store,
then implement the JdbcTemplate-based class in infrastructure that implements
the IdempotencyStore interface.
- Around line 129-155: reserveIdempotencyKey currently fails with
DataIntegrityViolationException if an expired row still exists (composite key
collision) even though lookupIdempotencyKey ignores expired rows; modify
reserveIdempotencyKey to, on DataIntegrityViolationException, delete any expired
record for the same idempotency_key/request_method/request_path in TABLE_NAME
(e.g. DELETE ... WHERE idempotency_key = ? AND request_method = ? AND
request_path = ? AND expires_at <= NOW()), then retry the INSERT once and return
true if it succeeds (otherwise return false); ensure you reference the existing
methods/variables reserveIdempotencyKey, lookupIdempotencyKey and TABLE_NAME
when locating where to add the delete-and-retry logic.
- Around line 92-100: The current reserved idempotency row can remain in pending
state if chain.doFilter or finalizeIdempotencyKey throws; wrap the replayable
request handling (the code using reserveIdempotencyKey, new
CachedBodyRequestWrapper, new ContentCachingResponseWrapper, chain.doFilter, and
finalizeIdempotencyKey) in a try/catch/finally block so that on any exception
you either delete the pending reservation or persist a terminal error response
via the same finalizeIdempotencyKey path (or a dedicated deleteReservation
method) before rethrowing the exception; ensure
wrappedResponse.copyBodyToResponse() still runs on success and the exception is
rethrown after cleanup.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/job/IdempotencyCleanupJob.java`:
- Around line 16-24: The IdempotencyCleanupJob currently directly uses
JdbcTemplate and SQL; instead define a domain port interface (e.g.,
IdempotencyCleanupPort with a method like deleteExpiredBatch() returning int or
deleteExpiredKeysBatch()) and inject that port into IdempotencyCleanupJob
(replace direct JdbcTemplate usage so cleanExpiredKeys() calls the port in its
loop). Move the JdbcTemplate-based implementation into the infrastructure layer
as an adapter (e.g., JdbcIdempotencyCleanupAdapter) that implements the new port
and contains the SQL and JdbcTemplate usage, and register that adapter as the
concrete bean to be injected into the job.
In
`@merchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V17__scope_idempotency_keys.sql`:
- Line 1: Add a short SQL comment immediately above the DROP line explaining why
dropping onboarding_idempotency_keys_pkey is required and safe in this migration
(e.g., being replaced by a different constraint, key rename, or schema change
and no live data loss expected), referencing the table
onboarding_idempotency_keys and the migration V17__scope_idempotency_keys.sql;
ensure the comment uses SQL single-line comment syntax (--) and briefly states
the rationale and any cross-reference to the new constraint or prior migration
that makes the destructive operation safe.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-125: The test should assert observable behavior: after
stubbing reserveIdempotencyKey to return true in
shouldProceedAndFinalize_whenReservationSucceeds, verify that
filterChain.doFilter(request, response) was invoked and that the spy filter's
finalizeIdempotencyKey was called for the reserved key; replace the lone status
assertion with Mockito.verify(filterChain, times(1)).doFilter(request, response)
and Mockito.verify(filter).finalizeIdempotencyKey(eq("key-789"), any()) (or
match the exact second arg your finalizeIdempotencyKey method expects) to ensure
the chain was executed and the idempotency record was finalized.
---
Duplicate comments:
In
`@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.java`:
- Around line 32-142: Add a new concurrent integration test to
IdempotencyKeyFilterIT that races two POSTs with the same Idempotency-Key to
MERCHANT_ENDPOINT: create a test method (e.g.,
shouldHandleConcurrentDuplicateRequests) that uses ExecutorService (or
CompletableFuture) and a CountDownLatch to start two mockMvc.post calls
simultaneously with the same idempotencyKey and identical requestBody, collect
both MockHttpServletResponses, and assert that exactly one request resulted in a
successful create (status 201) while the other resulted in either a replay
(status 201 with header "Idempotency-Replay" == "true") or a conflict (status
409); finally use jdbcTemplate.queryForObject to assert the
gatewayiam_idempotency_keys row count for that idempotencyKey (and same
request_hash/composite key) equals 1. Ensure you reference MERCHANT_ENDPOINT,
mockMvc, jdbcTemplate, and the idempotency key/request body variables when
adding the test.
In
`@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-125: The test should assert side effects beyond the 200
status: after calling filter.doFilterInternal in
shouldProceedAndFinalize_whenReservationSucceeds, verify that
filterChain.doFilter(request, response) was invoked to ensure the request
reached the chain, and verify that the idempotency reservation was finalized by
asserting a call to the finalize/update method (e.g., finalizeIdempotencyKey or
finalizeReservation) with the expected key "key-789" and request metadata; keep
the existing use of reserveIdempotencyKey("key-789", "POST", "/v1/merchants",
REQUEST_HASH) and add verify(filterChain).doFilter(...) and verify(...) for the
finalization call so the test fails if the happy-path finalization regresses.
In
`@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.java`:
- Around line 107-118: The test currently constructs a new IdempotencyCleanupJob
instance directly (new
com.stablecoin.payments.merchant.onboarding.application.job.IdempotencyCleanupJob(jdbcTemplate)),
which bypasses Spring wiring and the `@ConditionalOnProperty` behavior; instead,
autowire the Spring-managed bean of type
com.stablecoin.payments.merchant.onboarding.application.job.IdempotencyCleanupJob
into the test (e.g., add an `@Autowired` field idempotencyCleanupJob) and call
idempotencyCleanupJob.cleanExpiredKeys() so the test exercises the actual
scheduled/conditional bean from the application context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ed9862b-defb-40e7-abaf-ad9c1475a320
📒 Files selected for processing (15)
api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterIT.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/job/IdempotencyCleanupJob.javaapi-gateway-iam/api-gateway-iam/src/main/resources/db/migration/V9__scope_idempotency_keys.sqlapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterIT.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/job/IdempotencyCleanupJob.javamerchant-iam/merchant-iam/src/main/resources/db/migration/V11__scope_idempotency_keys.sqlmerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterIT.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/job/IdempotencyCleanupJob.javamerchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V17__scope_idempotency_keys.sqlmerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java
Comment on lines
+159
to
+178
| void finalizeIdempotencyKey(String key, String method, String path, | ||
| ContentCachingResponseWrapper wrappedResponse) { | ||
| var responseBody = new String(wrappedResponse.getContentAsByteArray(), StandardCharsets.UTF_8); | ||
| var statusCode = wrappedResponse.getStatus(); | ||
| var expiresAt = Timestamp.from(Instant.now().plus(ttlHours, ChronoUnit.HOURS)); | ||
|
|
||
| jdbcTemplate.update( | ||
| "UPDATE " + TABLE_NAME | ||
| + " SET response_body = ?, status_code = ?, expires_at = ?" | ||
| + " WHERE idempotency_key = ? AND request_method = ? AND request_path = ?", | ||
| responseBody, statusCode, expiresAt, key, method, path); | ||
| } | ||
|
|
||
| private void replayResponse(HttpServletResponse response, IdempotencyRecord record) | ||
| throws IOException { | ||
| log.info("Replaying idempotent response"); | ||
| response.setStatus(record.statusCode()); | ||
| response.setContentType(MediaType.APPLICATION_JSON_VALUE); | ||
| response.setHeader(IDEMPOTENCY_REPLAY_HEADER, "true"); | ||
| response.getWriter().write(record.responseBody()); |
There was a problem hiding this comment.
Replay must preserve the original response metadata.
Only response_body and status_code are stored, then replay hard-codes application/json. For any mutation that returns a different media type or depends on headers like Location, the replayed response no longer matches the original. Persist the content type and replay-relevant headers with the record, then restore them here; otherwise scope this filter to JSON-only endpoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java`
around lines 159 - 178, The replay currently hardcodes
MediaType.APPLICATION_JSON and only stores response_body and status_code; update
finalizeIdempotencyKey to also persist the response content type and any
replay-relevant headers (e.g., Content-Type, Location, Cache-Control) into the
idempotency record (extend TABLE_NAME schema and the JDBC UPDATE to include
content_type and headers), update the IdempotencyRecord to hold contentType and
a map/string of headers, and modify replayResponse to restore
response.setContentType(record.contentType()) and iterate/restore the stored
headers (and remove the hard-coded application/json) before writing
record.responseBody(); ensure the JDBC read path that builds IdempotencyRecord
populates these new fields.
| @@ -0,0 +1,6 @@ | |||
| ALTER TABLE gatewayiam_idempotency_keys DROP CONSTRAINT gatewayiam_idempotency_keys_pkey; | |||
There was a problem hiding this comment.
Add a justification comment for the PK drop.
This migration changes a live constraint, but the script does not explain why the destructive step is required or why it is safe for existing rows. Add a short SQL comment before the DROP CONSTRAINT.
As per coding guidelines, "Destructive operations (DROP, TRUNCATE, DELETE without WHERE) must have a justification comment".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api-gateway-iam/api-gateway-iam/src/main/resources/db/migration/V9__scope_idempotency_keys.sql`
at line 1, Add a short SQL comment immediately above the ALTER TABLE statement
explaining why the primary key is being dropped and why the operation is safe
for existing rows; reference the gatewayiam_idempotency_keys table and the
gatewayiam_idempotency_keys_pkey constraint in the comment and state the
justification (e.g., replacing PK with new constraint, fixing schema mismatch,
or non-destructive migration validated against existing data) so reviewers can
understand the destructive change before the ALTER TABLE DROP CONSTRAINT
gatewayiam_idempotency_keys_pkey line.
Comment on lines
+97
to
+98
| var bodyBytes = request.getInputStream().readAllBytes(); | ||
| var requestHash = computeSha256(bodyBytes); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for request body size limits in application configuration
fd -e yml -e yaml -e properties | xargs grep -l -E '(max-http-post-size|max-swallow-size|max-request-size|maxPostSize)' 2>/dev/null || echo "No explicit size limits found in config files"Repository: Puneethkumarck/stablebridge-platform
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Find application configuration files
fd -e properties -e yml -e yaml merchant-iam | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Search for Spring Boot configuration properties (including application.properties/yml)
find merchant-iam -type f \( -name "application*.properties" -o -name "application*.yml" -o -name "application*.yaml" \)Repository: Puneethkumarck/stablebridge-platform
Length of output: 375
🏁 Script executed:
#!/bin/bash
# Look at the IdempotencyKeyFilter implementation to understand scope and body handling
cat -n merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java | sed -n '74,140p'Repository: Puneethkumarck/stablebridge-platform
Length of output: 3353
🏁 Script executed:
#!/bin/bash
# Check if there's a custom ServletConfiguration or FilterConfig
fd -e java merchant-iam | xargs grep -l "EmbeddedServletContainerCustomizer\|TomcatServletWebServerFactory\|ConfigurableServletWebServerFactory\|server\.tomcat" 2>/dev/null | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check the main application configuration for size limits
cat merchant-iam/merchant-iam/src/main/resources/application.ymlRepository: Puneethkumarck/stablebridge-platform
Length of output: 3872
🏁 Script executed:
#!/bin/bash
# Also check local application config
cat merchant-iam/merchant-iam/src/main/resources/application-local.ymlRepository: Puneethkumarck/stablebridge-platform
Length of output: 510
🏁 Script executed:
#!/bin/bash
# Search for MUTATING_METHODS and EXEMPT_PREFIXES constant definitions to understand filter scope
cat -n merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java | sed -n '1,73p' | tail -50Repository: Puneethkumarck/stablebridge-platform
Length of output: 2382
Add explicit request body size validation to prevent DoS via unbounded readAllBytes().
The filter applies to all state-mutating endpoints (POST, PUT, PATCH, DELETE) on payment and management resources — excluding only /v1/auth/*, /v1/invitations/*, and /actuator/*. Without an upstream size limit, attackers can exhaust heap memory by sending multi-GB payloads.
The application configuration contains no server.tomcat.max-http-post-size or spring.servlet.multipart.max-request-size settings; Spring's defaults may be unlimited depending on version. Add explicit validation:
-
Configure Tomcat limit in
application.yml:server: tomcat: max-http-post-size: 10485760 # 10 MB
-
Or add programmatic check in the filter before line 97:
if (request.getContentLength() > MAX_BODY_SIZE) { response.setStatus(HttpStatus.PAYLOAD_TOO_LARGE.value()); // error response return; }
Given this is merchant-iam (security-critical service), add guardrails immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.java`
around lines 97 - 98, The filter currently calls
request.getInputStream().readAllBytes() (seen near computeSha256 in
IdempotencyKeyFilter) without bounding the body size; add a guard to prevent DoS
by either configuring Tomcat max-http-post-size in application.yml (e.g. 10MB)
or, immediately before reading the stream in IdempotencyKeyFilter, check the
Content-Length/request.getContentLength() against a MAX_BODY_SIZE constant and
if exceeded set HttpStatus.PAYLOAD_TOO_LARGE on the response and return; ensure
MAX_BODY_SIZE is defined and used consistently so computeSha256 is only invoked
on safely sized payloads.
Comment on lines
+16
to
+24
| private final JdbcTemplate jdbcTemplate; | ||
|
|
||
| @Scheduled(cron = "${app.idempotency.cleanup-cron:0 0 * * * *}") | ||
| public void cleanExpiredKeys() { | ||
| int totalDeleted = 0; | ||
| int deleted; | ||
| do { | ||
| deleted = jdbcTemplate.update( | ||
| "WITH expired AS (" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep cleanup storage behind a port.
This job is in the application layer, but it owns the JDBC dependency and SQL directly. Inject a cleanup port here and move the JdbcTemplate implementation into infrastructure/ so the scheduler only triggers the use case. As per coding guidelines, "All external calls go through ports (interfaces) in the domain layer" and "Adapters live in infrastructure/ and must implement domain ports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/job/IdempotencyCleanupJob.java`
around lines 16 - 24, The IdempotencyCleanupJob currently directly uses
JdbcTemplate and SQL; instead define a domain port interface (e.g.,
IdempotencyCleanupPort with a method like deleteExpiredBatch() returning int or
deleteExpiredKeysBatch()) and inject that port into IdempotencyCleanupJob
(replace direct JdbcTemplate usage so cleanExpiredKeys() calls the port in its
loop). Move the JdbcTemplate-based implementation into the infrastructure layer
as an adapter (e.g., JdbcIdempotencyCleanupAdapter) that implements the new port
and contains the SQL and JdbcTemplate usage, and register that adapter as the
concrete bean to be injected into the job.
| @@ -0,0 +1,6 @@ | |||
| ALTER TABLE onboarding_idempotency_keys DROP CONSTRAINT onboarding_idempotency_keys_pkey; | |||
There was a problem hiding this comment.
Add rationale comment before destructive constraint drop.
Line 1 drops a constraint with no justification in-script. Add a short SQL comment stating why this drop is required and safe for this migration step.
Suggested patch
+-- Replace single-column PK with endpoint-scoped composite PK for idempotency semantics.
ALTER TABLE onboarding_idempotency_keys DROP CONSTRAINT onboarding_idempotency_keys_pkey;As per coding guidelines, "Destructive operations (DROP, TRUNCATE, DELETE without WHERE) must have a justification comment".
📝 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.
Suggested change
| ALTER TABLE onboarding_idempotency_keys DROP CONSTRAINT onboarding_idempotency_keys_pkey; | |
| -- Replace single-column PK with endpoint-scoped composite PK for idempotency semantics. | |
| ALTER TABLE onboarding_idempotency_keys DROP CONSTRAINT onboarding_idempotency_keys_pkey; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/main/resources/db/migration/V17__scope_idempotency_keys.sql`
at line 1, Add a short SQL comment immediately above the DROP line explaining
why dropping onboarding_idempotency_keys_pkey is required and safe in this
migration (e.g., being replaced by a different constraint, key rename, or schema
change and no live data loss expected), referencing the table
onboarding_idempotency_keys and the migration V17__scope_idempotency_keys.sql;
ensure the comment uses SQL single-line comment syntax (--) and briefly states
the rationale and any cross-reference to the new constraint or prior migration
that makes the destructive operation safe.
Comment on lines
+114
to
+125
| @Test | ||
| @DisplayName("should proceed with chain when reservation succeeds") | ||
| void shouldProceedAndFinalize_whenReservationSucceeds() throws ServletException, IOException { | ||
| request.addHeader("Idempotency-Key", "key-789"); | ||
| request.setContent(REQUEST_BODY.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| doReturn(true).when(filter).reserveIdempotencyKey("key-789", "POST", "/api/v1/merchants", REQUEST_HASH); | ||
|
|
||
| filter.doFilterInternal(request, response, filterChain); | ||
|
|
||
| assertThat(response.getStatus()).isEqualTo(200); | ||
| } |
There was a problem hiding this comment.
Make the reservation-success test assert real behavior.
Line 124 only checks the default MockHttpServletResponse status, so this still passes if the filter never calls the chain or never finalizes the record. Verify filterChain.doFilter(...) and finalizeIdempotencyKey(...) on the spy instead. As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java`
around lines 114 - 125, The test should assert observable behavior: after
stubbing reserveIdempotencyKey to return true in
shouldProceedAndFinalize_whenReservationSucceeds, verify that
filterChain.doFilter(request, response) was invoked and that the spy filter's
finalizeIdempotencyKey was called for the reserved key; replace the lone status
assertion with Mockito.verify(filterChain, times(1)).doFilter(request, response)
and Mockito.verify(filter).finalizeIdempotencyKey(eq("key-789"), any()) (or
match the exact second arg your finalizeIdempotencyKey method expects) to ensure
the chain was executed and the idempotency record was finalized.
…execution Testcontainers started in static initializers were never stopped, causing orphaned PostgreSQL, Kafka, Redis, and Mailpit containers to accumulate after each test run (~30-50 containers, 5-15 GB leaked memory). Add Runtime.getRuntime().addShutdownHook() to all 10 AbstractIntegrationTest classes to ensure containers are stopped when the JVM exits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…afety, expired reclaim (STA-72) - Only persist idempotency records for 2xx responses; delete reservation for non-2xx so failed requests can be retried - Wrap chain.doFilter in try-catch to clean up reservation on exception, preventing orphaned in-flight rows that block retries until TTL expires - Reclaim expired rows on INSERT conflict: if cleanup hasn't run and an expired row blocks the INSERT, delete it and retry once - Add PUT method unit test to all 3 services - Strengthen ProceedAndPersist test assertion to verify no error codes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java (1)
114-127:⚠️ Potential issue | 🟡 MinorTest asserts MockHttpServletResponse default rather than filter behavior.
Same issue as other modules. Replace status assertion with verification that
filterChain.doFilterwas invoked.As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java` around lines 114 - 127, The test should verify the filter forwarded the request instead of asserting MockHttpServletResponse defaults; in the test method shouldProceedAndFinalize_whenReservationSucceeds() replace the assertion assertThat(response.getStatus()).isEqualTo(200) with a verification that filterChain.doFilter(request, response) was invoked (e.g., verify(filterChain).doFilter(request, response)), keeping the existing check that the response body does not contain reserved keys and retaining the reservation stub doReturn(...) for reserveIdempotencyKey.merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.java (1)
114-127:⚠️ Potential issue | 🟡 MinorTest asserts MockHttpServletResponse default rather than filter behavior.
Line 124 asserts
response.getStatus() == 200, but sincefilterChainis mocked and does nothing, this isMockHttpServletResponse's default value. The test passes even if the filter never calls the chain.Add verification that
filterChain.doFilterwas invoked:filter.doFilterInternal(request, response, filterChain); - assertThat(response.getStatus()).isEqualTo(200); - assertThat(response.getContentAsString()).doesNotContain("IAM-0001", "IAM-0002", "IAM-0003"); + then(filterChain).should().doFilter( + org.mockito.ArgumentMatchers.any(), + org.mockito.ArgumentMatchers.any());As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.java` around lines 114 - 127, The test should verify that the filter actually invoked the chain rather than relying on MockHttpServletResponse defaults: in shouldProceedAndFinalize_whenReservationSucceeds (class IdempotencyKeyFilterTest) after calling filter.doFilterInternal(request, response, filterChain), add a Mockito verification that filterChain.doFilter(request, response) was called (e.g., verify(filterChain).doFilter(request, response)); keep the existing assertions about response content but replace or augment the status assertion with the doFilter verification to ensure the filter proceeds with the chain when reserveIdempotencyKey returns true.api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java (1)
114-127:⚠️ Potential issue | 🟡 MinorTest asserts MockHttpServletResponse default rather than filter behavior.
Same issue as merchant-iam: Line 124 checks default status. Verify
filterChain.doFilterwas invoked to confirm the filter proceeded correctly.As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java` around lines 114 - 127, The test should verify the filter actually forwarded the request instead of asserting MockHttpServletResponse's default status; in the test method shouldProceedAndFinalize_whenReservationSucceeds(), after calling filter.doFilterInternal(...) replace or supplement the response status assertion with a Mockito verification that filterChain.doFilter(request, response) was invoked (and optionally verify finalizeIdempotencyKey was called), keeping the existing mock setup of reserveIdempotencyKey("key-789", "POST", "/v1/merchants", REQUEST_HASH) to ensure the reservation path is exercised.api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java (1)
206-213: 🧹 Nitpick | 🔵 TrivialReplay hardcodes
application/jsonContent-Type.The
replayResponsemethod setsMediaType.APPLICATION_JSON_VALUEregardless of the original response's Content-Type. If any endpoint returns XML, plain text, or other formats, the replayed response will have incorrect metadata.For a JSON-only API gateway this may be acceptable; otherwise, persist and restore the original Content-Type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java` around lines 206 - 213, The replayResponse method currently hardcodes MediaType.APPLICATION_JSON_VALUE; update it to restore the original Content-Type stored in the IdempotencyRecord instead: modify IdempotencyRecord to persist the response content type (e.g., add a contentType() field) when saving responses in the idempotency flow, then change replayResponse(HttpServletResponse response, IdempotencyRecord record) to set the response Content-Type from record.contentType() (falling back to a sensible default such as MediaType.APPLICATION_JSON_VALUE only if contentType is null/empty), and keep setting status, body and IDEMPOTENCY_REPLAY_HEADER as before so replayed responses preserve their original media type.merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.java (1)
57-64: 🛠️ Refactor suggestion | 🟠 MajorJDBC coupling violates hexagonal architecture.
IdempotencyKeyFilterdirectly usesJdbcTemplate, coupling the HTTP adapter to persistence details. Extract anIdempotencyStoreport interface in the domain layer with a JDBC adapter implementation ininfrastructure/.As per coding guidelines, "All external calls go through ports (interfaces) in the domain layer" and "Adapters live in infrastructure/ and must implement domain ports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.java` around lines 57 - 64, IdempotencyKeyFilter currently depends on JdbcTemplate, coupling the HTTP adapter to persistence; extract a domain port interface (e.g., IdempotencyStore with methods like exists(key) and record(key, ttlHours)) and update IdempotencyKeyFilter to depend on that interface instead of JdbcTemplate (preserve the ttlHours constructor param). Implement a JDBC adapter class in infrastructure (e.g., JdbcIdempotencyStore) that implements IdempotencyStore and uses JdbcTemplate for storage; wire the adapter into the application config so IdempotencyKeyFilter receives the IdempotencyStore implementation at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`:
- Around line 33-36: The shutdown hook currently calls KAFKA.stop() then
POSTGRES.stop() sequentially so an exception in KAFKA.stop() prevents
POSTGRES.stop() from running; modify the lambda passed to
Runtime.getRuntime().addShutdownHook to wrap each stop call in its own try/catch
(e.g., try { KAFKA.stop(); } catch (Exception e) { /* log error */ } and
likewise for POSTGRES.stop()) so both containers get best-effort teardown and
exceptions are logged/handled without aborting the remaining cleanup.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.java`:
- Around line 33-36: The shutdown hook currently calls KAFKA.stop() and
POSTGRES.stop() sequentially so a failure in KAFKA.stop() prevents
POSTGRES.stop() from running; update the
Runtime.getRuntime().addShutdownHook(...) lambda to call each stop in its own
try/catch (or equivalent protective block) so that KAFKA.stop() errors are
caught and logged but do not prevent POSTGRES.stop() from executing, and vice
versa; reference the existing KAFKA.stop() and POSTGRES.stop() calls inside the
shutdown hook and ensure any exceptions are handled (logged via your test logger
or System.err) so both resources are always attempted to be stopped.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java`:
- Around line 33-36: The shutdown hook uses Runtime.getRuntime().addShutdownHook
with a lambda that calls KAFKA.stop() and POSTGRES.stop() sequentially, but an
exception in KAFKA.stop() will prevent POSTGRES.stop() from running; modify the
shutdown hook to defensively handle errors by wrapping each stop call in
try/catch (or use a try { KAFKA.stop(); } finally { try { POSTGRES.stop(); }
catch (...) { ... } }) so both KAFKA.stop() and POSTGRES.stop() are guaranteed
to be attempted and any exceptions are logged rather than aborting the remaining
teardown.
In
`@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java`:
- Around line 33-36: The shutdown hook currently calls KAFKA.stop() and
POSTGRES.stop() directly; wrap each call in its own try-catch inside the
Runtime.getRuntime().addShutdownHook(new Thread(...)) so that if KAFKA.stop()
throws it won’t prevent POSTGRES.stop() from running; catch Throwable (or
Exception) for each and log the error/exception (using your test logger or
System.err) with context indicating which stop failed.
In
`@merchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.java`:
- Around line 53-58: The shutdown hook added via
Runtime.getRuntime().addShutdownHook currently calls REDIS.stop(),
MAILPIT.stop(), KAFKA.stop(), and POSTGRES.stop() in sequence so an exception in
any stop() aborts the rest; modify the hook to call each container's stop()
inside its own try/catch (catch Throwable) and log the error (e.g., process or
test logger) so a failure in REDIS.stop() won’t prevent MAILPIT.stop(),
KAFKA.stop(), or POSTGRES.stop() from running; keep the same identifiers
(Runtime.getRuntime().addShutdownHook, REDIS, MAILPIT, KAFKA, POSTGRES, stop())
to locate and update the code.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java`:
- Around line 184-192: The computeHash helper in IdempotencyKeyFilterTest is
duplicated across tests; extract it into a shared test utility (e.g., a new
class TestHashUtils with a public static computeHash(byte[])) and update
IdempotencyKeyFilterTest to call TestHashUtils.computeHash(...); ensure the new
utility implements the same logic (using MessageDigest.getInstance("SHA-256")
and HexFormat) and replace duplicate copies in the other two test files to
import and reuse this single method.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java`:
- Around line 34-37: The shutdown hook currently calls KAFKA.stop() then
POSTGRES.stop() directly inside a single lambda so if KAFKA.stop() throws the
POSTGRES.stop() call is skipped; modify the
Runtime.getRuntime().addShutdownHook(new Thread(() -> { ... })) lambda to call
each stop inside its own try/catch (e.g., try { KAFKA.stop(); } catch (Throwable
t) { log/error with context } and then try { POSTGRES.stop(); } catch (Throwable
t) { log/error with context }) so both KAFKA and POSTGRES stop attempts always
run even if one fails, referencing the existing KAFKA and POSTGRES variables and
the shutdown hook thread.
---
Duplicate comments:
In
`@api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.java`:
- Around line 206-213: The replayResponse method currently hardcodes
MediaType.APPLICATION_JSON_VALUE; update it to restore the original Content-Type
stored in the IdempotencyRecord instead: modify IdempotencyRecord to persist the
response content type (e.g., add a contentType() field) when saving responses in
the idempotency flow, then change replayResponse(HttpServletResponse response,
IdempotencyRecord record) to set the response Content-Type from
record.contentType() (falling back to a sensible default such as
MediaType.APPLICATION_JSON_VALUE only if contentType is null/empty), and keep
setting status, body and IDEMPOTENCY_REPLAY_HEADER as before so replayed
responses preserve their original media type.
In
`@api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-127: The test should verify the filter actually forwarded the
request instead of asserting MockHttpServletResponse's default status; in the
test method shouldProceedAndFinalize_whenReservationSucceeds(), after calling
filter.doFilterInternal(...) replace or supplement the response status assertion
with a Mockito verification that filterChain.doFilter(request, response) was
invoked (and optionally verify finalizeIdempotencyKey was called), keeping the
existing mock setup of reserveIdempotencyKey("key-789", "POST", "/v1/merchants",
REQUEST_HASH) to ensure the reservation path is exercised.
In
`@merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-127: The test should verify that the filter actually invoked
the chain rather than relying on MockHttpServletResponse defaults: in
shouldProceedAndFinalize_whenReservationSucceeds (class
IdempotencyKeyFilterTest) after calling filter.doFilterInternal(request,
response, filterChain), add a Mockito verification that
filterChain.doFilter(request, response) was called (e.g.,
verify(filterChain).doFilter(request, response)); keep the existing assertions
about response content but replace or augment the status assertion with the
doFilter verification to ensure the filter proceeds with the chain when
reserveIdempotencyKey returns true.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.java`:
- Around line 57-64: IdempotencyKeyFilter currently depends on JdbcTemplate,
coupling the HTTP adapter to persistence; extract a domain port interface (e.g.,
IdempotencyStore with methods like exists(key) and record(key, ttlHours)) and
update IdempotencyKeyFilter to depend on that interface instead of JdbcTemplate
(preserve the ttlHours constructor param). Implement a JDBC adapter class in
infrastructure (e.g., JdbcIdempotencyStore) that implements IdempotencyStore and
uses JdbcTemplate for storage; wire the adapter into the application config so
IdempotencyKeyFilter receives the IdempotencyStore implementation at runtime.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java`:
- Around line 114-127: The test should verify the filter forwarded the request
instead of asserting MockHttpServletResponse defaults; in the test method
shouldProceedAndFinalize_whenReservationSucceeds() replace the assertion
assertThat(response.getStatus()).isEqualTo(200) with a verification that
filterChain.doFilter(request, response) was invoked (e.g.,
verify(filterChain).doFilter(request, response)), keeping the existing check
that the response body does not contain reserved keys and retaining the
reservation stub doReturn(...) for reserveIdempotencyKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc368a96-a6d6-41b5-9334-bf861e7f390d
📒 Files selected for processing (16)
api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilter.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/application/config/IdempotencyKeyFilterTest.javablockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javacompliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.javafiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.javafx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.javaledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilter.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/application/config/IdempotencyKeyFilterTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilter.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java
Comment on lines
+33
to
+36
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect shutdown hook and teardown calls with context
rg -n -C3 'addShutdownHook|\.stop\(\)' blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 307
🏁 Script executed:
cat -n blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 3018
🏁 Script executed:
# Search for other lifecycle methods in integration tests
rg -n '@AfterAll|@After|@TearDown' blockchain-custody/blockchain-custody/src/integration-test/java/ -t javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
# Check if there are other shutdown hook patterns in the codebase
rg -n 'addShutdownHook' blockchain-custody/ -t javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 271
Add exception isolation to container teardown.
The static shutdown hook sequentially stops Kafka then PostgreSQL without exception handling. If KAFKA.stop() throws, POSTGRES.stop() skips, leaving the container running. Wrap each stop call independently to ensure best-effort cleanup:
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- KAFKA.stop();
- POSTGRES.stop();
+ try {
+ KAFKA.stop();
+ } catch (Exception ignored) {
+ // best-effort teardown
+ }
+ try {
+ POSTGRES.stop();
+ } catch (Exception ignored) {
+ // best-effort teardown
+ }
}));📝 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.
Suggested change
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| KAFKA.stop(); | |
| POSTGRES.stop(); | |
| })); | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| try { | |
| KAFKA.stop(); | |
| } catch (Exception ignored) { | |
| // best-effort teardown | |
| } | |
| try { | |
| POSTGRES.stop(); | |
| } catch (Exception ignored) { | |
| // best-effort teardown | |
| } | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`
around lines 33 - 36, The shutdown hook currently calls KAFKA.stop() then
POSTGRES.stop() sequentially so an exception in KAFKA.stop() prevents
POSTGRES.stop() from running; modify the lambda passed to
Runtime.getRuntime().addShutdownHook to wrap each stop call in its own try/catch
(e.g., try { KAFKA.stop(); } catch (Exception e) { /* log error */ } and
likewise for POSTGRES.stop()) so both containers get best-effort teardown and
exceptions are logged/handled without aborting the remaining cleanup.
Comment on lines
+33
to
+36
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
Prevent partial teardown in shutdown hook
If KAFKA.stop() fails on Line 34, POSTGRES.stop() on Line 35 never executes. That can still orphan containers and degrade CI stability. Make each stop independent and guarded.
Proposed fix
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- KAFKA.stop();
- POSTGRES.stop();
- }));
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try {
+ if (KAFKA.isRunning()) {
+ KAFKA.stop();
+ }
+ } catch (Exception ignored) {
+ // best-effort cleanup
+ }
+ try {
+ if (POSTGRES.isRunning()) {
+ POSTGRES.stop();
+ }
+ } catch (Exception ignored) {
+ // best-effort cleanup
+ }
+ }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.java`
around lines 33 - 36, The shutdown hook currently calls KAFKA.stop() and
POSTGRES.stop() sequentially so a failure in KAFKA.stop() prevents
POSTGRES.stop() from running; update the
Runtime.getRuntime().addShutdownHook(...) lambda to call each stop in its own
try/catch (or equivalent protective block) so that KAFKA.stop() errors are
caught and logged but do not prevent POSTGRES.stop() from executing, and vice
versa; reference the existing KAFKA.stop() and POSTGRES.stop() calls inside the
shutdown hook and ensure any exceptions are handled (logged via your test logger
or System.err) so both resources are always attempted to be stopped.
Comment on lines
+33
to
+36
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 2919
🏁 Script executed:
# Check if isRunning() is a real method on Testcontainers Container
# Also search for other shutdown patterns in the codebase
rg "\.isRunning\(\)" --type javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
# Search for other shutdown hook patterns
rg "addShutdownHook|\.stop\(\)" --type java -B 2 -A 2Repository: Puneethkumarck/stablebridge-platform
Length of output: 18456
🏁 Script executed:
# Check if there are other integration test base classes with similar patterns
fd ".*IntegrationTest.*\.java" --type f | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 1426
🌐 Web query:
Testcontainers Container isRunning() method API
💡 Result:
In Testcontainers for Java, isRunning() is part of the org.testcontainers.containers.ContainerState API (which most containers like GenericContainer, PostgreSQLContainer, etc. implement).
- Signature:
default boolean isRunning() - Meaning: returns whether the container is currently running.
(Javadoc: “Returns: is the container currently running?”) [1]
Typical usage (JUnit 5 example):
assertThat(MY_SQL_CONTAINER.isRunning()).isTrue();[2]
Related checks in the same API: isCreated() and isHealthy() (healthcheck-based “healthy” state). [1]
Sources:
[1] Testcontainers Javadoc — ContainerState#isRunning()
[2] Testcontainers docs — JUnit 5 integration example using isRunning()
🌐 Web query:
Testcontainers best practices shutdown hook cleanup multiple containers
💡 Result:
Best practices for cleanup / shutdown with multiple Testcontainers
-
Rely on Ryuk (the Resource Reaper) for cleanup; don’t add your own JVM shutdown hook in most cases. Testcontainers starts a dedicated “Ryuk” container that tracks resources created in the test session and removes them when the run finishes (even if tests fail). Disabling it is generally discouraged. [1], [2]
-
Prefer framework-managed lifecycle (JUnit 5 / JUnit 4 integrations) over manual hooks.
With JUnit 5, define containers as@Containerfields under@Testcontainersso the extension starts/stops them for you; this scales cleanly to multiple containers. Even if you don’t explicitly stop, Ryuk still cleans up. [3] -
If you must manage lifecycle yourself (custom runner, main method, non-JUnit framework):
- Use explicit
start()/stop()(ortry-with-resourceswhere applicable) rather than shutdown hooks, so teardown happens deterministically at the end of the suite. [4] - Put teardown in a guaranteed “finally/after-suite” path (e.g.,
@AfterAll) rather than relying on JVM shutdown ordering.
- Use explicit
-
For multiple containers, start them together and stop them together (suite-scoped).
Common pattern: start all dependencies once in@BeforeAll, stop all in@AfterAll, instead of per-test (faster, fewer race conditions). [3] -
Be careful with “Reusable Containers”.
If you enabletestcontainers.reuse.enable=true, containers may intentionally survive beyond the JVM, and cleanup/networking expectations change; it’s experimental and not suited for CI. In that mode, shutdown hooks/Ryuk won’t behave like the normal “everything is removed” model. [5], [6]
Practical rule of thumb
- Normal CI / standard integration tests: don’t write shutdown hooks; keep Ryuk enabled; use
@Testcontainers+ multiple@Containerfields (or explicit start/stop in suite hooks). [2], [3], [4] - Local dev speedups with reuse: opt-in reuse locally only, and accept that cleanup becomes a manual/explicit concern. [5], [6]
Sources:
[1] Testcontainers (.NET) Resource Reaper (Ryuk) docs (dotnet.testcontainers.org)
[2] Testcontainers-go Resource cleanup / Ryuk info (overview + disable flag mention) (deepwiki.com)
[3] Testcontainers guide: JUnit 5 lifecycle + Ryuk cleanup (testcontainers.com)
[4] Testcontainers Java: Manual container lifecycle control (java.testcontainers.org)
[5] Testcontainers Java: Reusable containers (experimental) + cleanup caveats (java.testcontainers.org)
[6] Testcontainers newsletter: reuse not suited for CI + limitations (newsletter.testcontainers.com)
Citations:
- 1: https://dotnet.testcontainers.org/api/resource_reaper/?utm_source=openai
- 2: https://deepwiki.com/testcontainers/testcontainers-go/2.5-resource-cleanup?utm_source=openai
- 3: https://testcontainers.com/guides/testcontainers-container-lifecycle/?utm_source=openai
- 4: https://java.testcontainers.org/test_framework_integration/manual_lifecycle_control/?utm_source=openai
- 5: https://java.testcontainers.org/features/reuse/?utm_source=openai
- 6: https://newsletter.testcontainers.com/announcements/enable-reusable-containers-with-a-single-click?utm_source=openai
Wrap shutdown operations in try/finally to prevent partial teardown.
An exception in KAFKA.stop() (line 34) will prevent POSTGRES.stop() from executing. Use defensive error handling to ensure both containers are stopped.
Suggested fix
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- KAFKA.stop();
- POSTGRES.stop();
+ try {
+ KAFKA.stop();
+ } finally {
+ POSTGRES.stop();
+ }
}));📝 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.
Suggested change
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| KAFKA.stop(); | |
| POSTGRES.stop(); | |
| })); | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| try { | |
| KAFKA.stop(); | |
| } finally { | |
| POSTGRES.stop(); | |
| } | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java`
around lines 33 - 36, The shutdown hook uses
Runtime.getRuntime().addShutdownHook with a lambda that calls KAFKA.stop() and
POSTGRES.stop() sequentially, but an exception in KAFKA.stop() will prevent
POSTGRES.stop() from running; modify the shutdown hook to defensively handle
errors by wrapping each stop call in try/catch (or use a try { KAFKA.stop(); }
finally { try { POSTGRES.stop(); } catch (...) { ... } }) so both KAFKA.stop()
and POSTGRES.stop() are guaranteed to be attempted and any exceptions are logged
rather than aborting the remaining teardown.
Comment on lines
+33
to
+36
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider wrapping each stop() in try-catch for robustness.
If KAFKA.stop() throws unexpectedly, POSTGRES.stop() will be skipped. While Testcontainers' stop() is generally exception-safe, a defensive pattern ensures both containers attempt cleanup.
♻️ Proposed defensive shutdown hook
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- KAFKA.stop();
- POSTGRES.stop();
+ try {
+ KAFKA.stop();
+ } catch (Exception e) {
+ // log or ignore
+ }
+ try {
+ POSTGRES.stop();
+ } catch (Exception e) {
+ // log or ignore
+ }
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java`
around lines 33 - 36, The shutdown hook currently calls KAFKA.stop() and
POSTGRES.stop() directly; wrap each call in its own try-catch inside the
Runtime.getRuntime().addShutdownHook(new Thread(...)) so that if KAFKA.stop()
throws it won’t prevent POSTGRES.stop() from running; catch Throwable (or
Exception) for each and log the error/exception (using your test logger or
System.err) with context indicating which stop failed.
Comment on lines
+53
to
+58
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| REDIS.stop(); | ||
| MAILPIT.stop(); | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
Harden shutdown hook so one failed stop doesn’t skip the rest
At Line 53, a failure in any stop() call aborts the hook and can still leave orphaned containers.
Suggested fix
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- REDIS.stop();
- MAILPIT.stop();
- KAFKA.stop();
- POSTGRES.stop();
+ try { REDIS.stop(); } catch (Exception ignored) {}
+ try { MAILPIT.stop(); } catch (Exception ignored) {}
+ try { KAFKA.stop(); } catch (Exception ignored) {}
+ try { POSTGRES.stop(); } catch (Exception ignored) {}
}));📝 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.
Suggested change
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| REDIS.stop(); | |
| MAILPIT.stop(); | |
| KAFKA.stop(); | |
| POSTGRES.stop(); | |
| })); | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| try { REDIS.stop(); } catch (Exception ignored) {} | |
| try { MAILPIT.stop(); } catch (Exception ignored) {} | |
| try { KAFKA.stop(); } catch (Exception ignored) {} | |
| try { POSTGRES.stop(); } catch (Exception ignored) {} | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.java`
around lines 53 - 58, The shutdown hook added via
Runtime.getRuntime().addShutdownHook currently calls REDIS.stop(),
MAILPIT.stop(), KAFKA.stop(), and POSTGRES.stop() in sequence so an exception in
any stop() aborts the rest; modify the hook to call each container's stop()
inside its own try/catch (catch Throwable) and log the error (e.g., process or
test logger) so a failure in REDIS.stop() won’t prevent MAILPIT.stop(),
KAFKA.stop(), or POSTGRES.stop() from running; keep the same identifiers
(Runtime.getRuntime().addShutdownHook, REDIS, MAILPIT, KAFKA, POSTGRES, stop())
to locate and update the code.
Comment on lines
+184
to
+192
| private static String computeHash(byte[] body) { | ||
| try { | ||
| var digest = MessageDigest.getInstance("SHA-256"); | ||
| var hash = digest.digest(body); | ||
| return HexFormat.of().formatHex(hash); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new IllegalStateException("SHA-256 not available", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting computeHash to a shared test utility.
This helper is duplicated across all three test files. Extract to a shared test utility class to reduce maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/application/config/IdempotencyKeyFilterTest.java`
around lines 184 - 192, The computeHash helper in IdempotencyKeyFilterTest is
duplicated across tests; extract it into a shared test utility (e.g., a new
class TestHashUtils with a public static computeHash(byte[])) and update
IdempotencyKeyFilterTest to call TestHashUtils.computeHash(...); ensure the new
utility implements the same logic (using MessageDigest.getInstance("SHA-256")
and HexFormat) and replace duplicate copies in the other two test files to
import and reuse this single method.
Comment on lines
+34
to
+37
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); |
There was a problem hiding this comment.
Harden shutdown hook so both containers are always attempted.
On Line 35, if KAFKA.stop() throws, POSTGRES.stop() on Line 36 is skipped. That can still leave orphaned containers in CI. Wrap stop calls so each executes independently.
Proposed fix
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- KAFKA.stop();
- POSTGRES.stop();
- }));
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try {
+ KAFKA.stop();
+ } catch (Exception ignored) {
+ // best-effort cleanup during JVM shutdown
+ } finally {
+ try {
+ POSTGRES.stop();
+ } catch (Exception ignored) {
+ // best-effort cleanup during JVM shutdown
+ }
+ }
+ }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java`
around lines 34 - 37, The shutdown hook currently calls KAFKA.stop() then
POSTGRES.stop() directly inside a single lambda so if KAFKA.stop() throws the
POSTGRES.stop() call is skipped; modify the
Runtime.getRuntime().addShutdownHook(new Thread(() -> { ... })) lambda to call
each stop inside its own try/catch (e.g., try { KAFKA.stop(); } catch (Throwable
t) { log/error with context } and then try { POSTGRES.stop(); } catch (Throwable
t) { log/error with context }) so both KAFKA and POSTGRES stop attempts always
run even if one fails, referencing the existing KAFKA and POSTGRES variables and
the shutdown hook thread.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IdempotencyKeyFilterwith full idempotency: persist key + request hash + response, replay cached responses on duplicates{prefix}_idempotency_keystable in S10 (V8), S11 (V16), S13 (V10)ContentCachingResponseWrapperfor response captureDataIntegrityViolationExceptioncatch + re-readIdempotencyCleanupJobwith configurable cron for TTL-based expiry cleanupIdempotency-Replay: trueheader on replayed responses, 422 on key reuse with different bodyCloses STA-72
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Database
Tests
Chores