feat(s1): infrastructure persistence tests -- adapter + audit log ITs (STA-108)#109
Merged
Merged
Conversation
Puneethkumarck
changed the title
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds integration-test infrastructure (DB cleanup, Kafka test producer), extensive persistence integration tests for payments and audit logs, an application-integration-test.yml, a Flyway migration to change payments.version to BIGINT, and repository + adapter support for querying payments by senderId and state. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java`:
- Around line 39-51: The call to entityManager.clear() in the `@BeforeEach`
cleanDatabase() is ineffective because Spring injects a shared EntityManager
proxy; either remove the entityManager.clear() line to avoid a false sense of
clearing the persistence context, or ensure it runs inside the same
transaction/persistence-context used by the repositories (e.g., make
cleanDatabase() `@Transactional` and call entityManager.clear() after the
TRUNCATE). Update the cleanDatabase() method accordingly and keep references to
the existing method name cleanDatabase(), the `@BeforeEach` annotated setup, and
the entityManager.clear() invocation when making the change.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentAuditLogRepositoryIT.java`:
- Around line 52-55: The test currently ignores all Instant fields which hides
mismatches for occurredAt; remove the .ignoringFieldsOfTypes(Instant.class) call
in the assertion chain in PaymentAuditLogRepositoryIT and instead add a
type-specific comparator for Instant that compares the persisted and expected
Instants at the database precision (e.g., truncating both to the DB precision
such as ChronoUnit.MILLIS) via AssertJ's usingComparatorForType for
Instant.class applied on the existing .usingRecursiveComparison() so that
occurredAt is asserted at DB precision rather than being completely ignored.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`:
- Around line 58-62: The assertions currently ignore all Instant fields (via
ignoringFieldsOfTypes(Instant.class)) which hides timestamp mapping regressions;
update the assertions around adapter.findById(saved.paymentId()) and the other
similar assertions (the ones comparing saved) to explicitly assert the Instant
fields createdAt, updatedAt, and expiresAt using either (a)
truncated-to-DB-precision comparison (e.g., truncate both expected and actual to
milliseconds or the DB precision) or (b) a tolerant comparator that allows a
small delta, instead of ignoring Instant.class; keep the BigDecimal comparator
for amounts and preserve recursive comparison for other fields but remove
ignoringFieldsOfTypes(Instant.class) and add targeted comparisons for those
timestamp fields so drift/loss fails the test.
- Around line 166-175: The test should assert the actual persisted JSON column,
not just the mapped domain metadata; after saving via adapter.save(payment) (use
saved.paymentId()), query the persistence side directly (e.g., via JdbcTemplate
or the PaymentEntity repository/finder) to fetch the raw metadata column for
that payment id and assert it is non-null and equals "{}" (or otherwise the
exact expected empty JSON representation) instead of only asserting
found.get().metadata() isEmpty(); update
shouldPersistEmptyMetadataAsEmptyJsonObject to perform that direct DB assertion
using the payment id returned by adapter.save.
- Around line 264-275: The test currently saves an already-COMPLETED fixture
(aCompletedPayment()) so it never exercises state transitions; change it to
start from an initiated fixture (e.g., aInitiatedPayment()), persist that
initial save via adapter.save(payment), then perform and persist each transition
(mutate the Payment to the next states and call adapter.save(...) for each
transition) until the payment reaches PaymentState.COMPLETED, and finally assert
that the retrieved payment equals the final saved version and that saved.state()
== PaymentState.COMPLETED; update the method
shouldUpdatePaymentThroughFullHappyPath to use aInitiatedPayment(),
adapter.save(...) for each step, and the same recursive comparison assertions.
- Around line 107-127: The test is calling
jpaRepository.findBySenderIdAndState(...) directly which bypasses
PaymentPersistenceAdapter; add a findBySenderIdAndState(UUID senderId,
PaymentState state) method to the PaymentRepository interface and implement a
corresponding public method on PaymentPersistenceAdapter that delegates to
repository.findBySenderIdAndState, then update the test to call
adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED) (after saving
via adapter.save(payment1/2)) and assert the exact returned payments and their
PaymentState (not hasSizeGreaterThanOrEqualTo) to validate adapter
mapping/filtering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 430ab27f-3e2b-4ca0-ac94-d18be123e6ae
📒 Files selected for processing (6)
payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/config/TestKafkaConfig.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentAuditLogRepositoryIT.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.javapayment-orchestrator/payment-orchestrator/src/integration-test/resources/application-integration-test.ymlpayment-orchestrator/payment-orchestrator/src/main/resources/db/migration/V3__fix_version_column_type.sql
Comment on lines
+39
to
+51
| @Autowired | ||
| private EntityManager entityManager; | ||
|
|
||
| @BeforeEach | ||
| void cleanDatabase() { | ||
| jdbcTemplate.execute(""" | ||
| TRUNCATE TABLE | ||
| payment_audit_log, | ||
| payments, | ||
| orchestrator_outbox_record | ||
| CASCADE | ||
| """); | ||
| entityManager.clear(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
entityManager.clear() here likely is not clearing what this test expects.
Spring's shared EntityManager proxy delegates to the current transactional EntityManager and otherwise creates a new one per operation, so calling clear() in @BeforeEach is unlikely to evict the persistence context later used by repository calls unless the test itself is transactional. If the goal is to invalidate cached entities after the TRUNCATE, either do the clear inside the same active transaction or drop this line to avoid a false sense of isolation. That's an inference from Spring's shared EntityManager contract. (docs.spring.io)
🤖 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 39 - 51, The call to entityManager.clear() in the `@BeforeEach`
cleanDatabase() is ineffective because Spring injects a shared EntityManager
proxy; either remove the entityManager.clear() line to avoid a false sense of
clearing the persistence context, or ensure it runs inside the same
transaction/persistence-context used by the repositories (e.g., make
cleanDatabase() `@Transactional` and call entityManager.clear() after the
TRUNCATE). Update the cleanDatabase() method accordingly and keep references to
the existing method name cleanDatabase(), the `@BeforeEach` annotated setup, and
the entityManager.clear() invocation when making the change.
Comment on lines
+52
to
+55
| assertThat(auditLogRepository.findById(saved.getLogId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(expected); |
There was a problem hiding this comment.
Don’t exclude occurredAt from the repository round-trip assertion.
This still passes if occurredAt is truncated, defaulted, or mapped back incorrectly because every Instant is ignored. Compare it at database precision instead of dropping the type from the assertion.
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
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentAuditLogRepositoryIT.java`
around lines 52 - 55, The test currently ignores all Instant fields which hides
mismatches for occurredAt; remove the .ignoringFieldsOfTypes(Instant.class) call
in the assertion chain in PaymentAuditLogRepositoryIT and instead add a
type-specific comparator for Instant that compares the persisted and expected
Instants at the database precision (e.g., truncating both to the DB precision
such as ChronoUnit.MILLIS) via AssertJ's usingComparatorForType for
Instant.class applied on the existing .usingRecursiveComparison() so that
occurredAt is asserted at DB precision rather than being completely ignored.
Comment on lines
+58
to
+62
| assertThat(adapter.findById(saved.paymentId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(saved); |
There was a problem hiding this comment.
The round-trip assertions are blind to timestamp mapping regressions.
Ignoring every Instant means createdAt, updatedAt, and expiresAt can drift or be lost without failing this suite. For persistence tests, compare those fields with DB-precision truncation or a tolerant comparator instead of excluding the entire type.
As per coding guidelines, "Assert on meaningful values — avoid assertTrue(result != null)".
Also applies to: 71-75, 92-96, 238-242, 257-261, 270-274
🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java`
around lines 58 - 62, The assertions currently ignore all Instant fields (via
ignoringFieldsOfTypes(Instant.class)) which hides timestamp mapping regressions;
update the assertions around adapter.findById(saved.paymentId()) and the other
similar assertions (the ones comparing saved) to explicitly assert the Instant
fields createdAt, updatedAt, and expiresAt using either (a)
truncated-to-DB-precision comparison (e.g., truncate both expected and actual to
milliseconds or the DB precision) or (b) a tolerant comparator that allows a
small delta, instead of ignoring Instant.class; keep the BigDecimal comparator
for amounts and preserve recursive comparison for other fields but remove
ignoringFieldsOfTypes(Instant.class) and add targeted comparisons for those
timestamp fields so drift/loss fails the test.
Comment on lines
+107
to
+127
| @Test | ||
| @DisplayName("should find payments by sender id and state") | ||
| void shouldFindPaymentsBySenderIdAndState() { | ||
| var payment1 = Payment.initiate( | ||
| "idem-sender-1", CORRELATION_ID, SENDER_ID, RECIPIENT_ID, | ||
| SOURCE_AMOUNT, SOURCE_CURRENCY, TARGET_CURRENCY, US_TO_DE); | ||
| var payment2 = Payment.initiate( | ||
| "idem-sender-2", CORRELATION_ID, SENDER_ID, RECIPIENT_ID, | ||
| SOURCE_AMOUNT, SOURCE_CURRENCY, TARGET_CURRENCY, US_TO_DE); | ||
| adapter.save(payment1); | ||
| adapter.save(payment2); | ||
|
|
||
| var results = jpaRepository.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED); | ||
| assertThat(results).hasSizeGreaterThanOrEqualTo(2); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return empty list when no payments match sender and state") | ||
| void shouldReturnEmptyListWhenNoPaymentsMatchSenderAndState() { | ||
| var results = jpaRepository.findBySenderIdAndState(UUID.randomUUID(), PaymentState.COMPLETED); | ||
| assertThat(results).isEmpty(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether PaymentPersistenceAdapter exposes a sender/state query
# and where findBySenderIdAndState is currently used.
fd 'PaymentPersistenceAdapter.java' | while read -r file; do
echo "== $file =="
sed -n '1,260p' "$file"
done
echo
echo "== call sites for findBySenderIdAndState =="
rg -nP --type=java '\bfindBySenderIdAndState\s*\('Repository: Puneethkumarck/stablebridge-platform
Length of output: 2681
Add findBySenderIdAndState to PaymentRepository and test through the adapter.
The test calls jpaRepository.findBySenderIdAndState(...) directly, bypassing the PaymentPersistenceAdapter under test. The adapter doesn't expose this query, so any adapter mapping or filtering bugs won't be caught. If this query is required, add it to the PaymentRepository interface and delegate through the adapter. Assert on the exact returned payments and their states instead of hasSizeGreaterThanOrEqualTo(2).
🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java`
around lines 107 - 127, The test is calling
jpaRepository.findBySenderIdAndState(...) directly which bypasses
PaymentPersistenceAdapter; add a findBySenderIdAndState(UUID senderId,
PaymentState state) method to the PaymentRepository interface and implement a
corresponding public method on PaymentPersistenceAdapter that delegates to
repository.findBySenderIdAndState, then update the test to call
adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED) (after saving
via adapter.save(payment1/2)) and assert the exact returned payments and their
PaymentState (not hasSizeGreaterThanOrEqualTo) to validate adapter
mapping/filtering.
Comment on lines
+166
to
+175
| @Test | ||
| @DisplayName("should persist empty metadata as empty JSON object") | ||
| void shouldPersistEmptyMetadataAsEmptyJsonObject() { | ||
| var payment = anInitiatedPayment(); | ||
| var saved = adapter.save(payment); | ||
|
|
||
| var found = adapter.findById(saved.paymentId()); | ||
| assertThat(found).isPresent(); | ||
| assertThat(found.get().metadata()).isEmpty(); | ||
| } |
There was a problem hiding this comment.
This does not verify that {} was persisted.
The current check only proves the adapter returns an empty map on read. It still passes if the column is stored as NULL and normalized during mapping. If the contract is “empty JSON object”, assert the raw persisted value, or at least the entity field before domain mapping, is non-null and empty.
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
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`
around lines 166 - 175, The test should assert the actual persisted JSON column,
not just the mapped domain metadata; after saving via adapter.save(payment) (use
saved.paymentId()), query the persistence side directly (e.g., via JdbcTemplate
or the PaymentEntity repository/finder) to fetch the raw metadata column for
that payment id and assert it is non-null and equals "{}" (or otherwise the
exact expected empty JSON representation) instead of only asserting
found.get().metadata() isEmpty(); update
shouldPersistEmptyMetadataAsEmptyJsonObject to perform that direct DB assertion
using the payment id returned by adapter.save.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java (3)
121-123:⚠️ Potential issue | 🟡 MinorAssert the exact rows returned by
findBySenderIdAndState.A size-only check will miss state-filtering or mapping regressions as long as two rows come back. Assert the returned IDs and
PaymentStatevalues, order-agnostic.Suggested assertion tightening
var results = adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED); - assertThat(results).hasSize(2); + assertThat(results) + .extracting(Payment::paymentId) + .containsExactlyInAnyOrder(payment1.paymentId(), payment2.paymentId()); + assertThat(results) + .allMatch(payment -> payment.state() == PaymentState.INITIATED);🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java` around lines 121 - 123, Replace the weak size-only assertion on the results from adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED) with an order-agnostic assertion that the exact rows are returned: assert that the collection contains the expected payment IDs and that each returned Payment has PaymentState.INITIATED (e.g., by extracting id and state from results and comparing as sets or comparing a Set of ids and verifying all states equal PaymentState.INITIATED), referencing the results variable and the adapter.findBySenderIdAndState call to locate the code.
60-64:⚠️ Potential issue | 🟡 MinorStop excluding all
Instantfields from the round-trip assertions.These assertions still pass if
createdAt,updatedAt, orexpiresAtare truncated, shifted, or dropped. Compare those fields explicitly at DB precision instead ofignoringFieldsOfTypes(Instant.class).Also applies to: 73-77, 94-98, 240-244, 259-263, 300-304
🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java` around lines 60 - 64, The assertion currently ignores all Instant fields in the round-trip check (the adapter.findById(...).isPresent().get() assertion chain), which hides DB-level truncation or loss of createdAt/updatedAt/expiresAt; remove ignoringFieldsOfTypes(Instant.class) and instead assert those Instant fields explicitly by comparing them at DB precision (e.g., truncate or round both saved and returned createdAt/updatedAt/expiresAt to the database precision) within the same recursive comparison or by separate assertions; apply the same change to the other occurrences mentioned (the similar chains at the other ranges) so each Instant field is compared deterministically rather than excluded.
168-176:⚠️ Potential issue | 🟠 MajorVerify the raw JSONB value for empty metadata.
This only proves the mapper returns an empty map. It still passes if the column is stored as
NULLand normalized on read. Please assert the persisted metadata column or entity field is non-null and represents an empty JSON object for the saved row.🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java` around lines 168 - 176, The test currently only checks the returned object's metadata map; instead assert the persisted raw JSONB column is non-null and equals an empty JSON object: after calling adapter.save(payment) (in PaymentPersistenceAdapterIT) flush or use the same persistence layer (e.g., entityManager, jdbcTemplate, or repository) to query the saved row for saved.paymentId() and read the metadata column value directly, then assert the column is not NULL and its string/value equals "{}" (or an empty JSON object) in addition to the existing mapper assertion. Use the concrete persistence accessor available in the test (entityManager/ jdbcTemplate/ repository) to fetch the raw column for verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`:
- Around line 181-205: The test currently exercises JPA directly instead of the
adapter, so update it to reproduce the optimistic-lock conflict through the
adapter: build a stale domain aggregate (use mapper.toEntity or create a domain
Payment with the old version), set its version to staleVersion and a new state
(FAILED), clear the EntityManager, then call
PaymentPersistenceAdapter.save(staleAggregate) (adapter.save(...)) and assert
that it throws ObjectOptimisticLockingFailureException; this ensures the broken
path in PaymentPersistenceAdapter.save (which currently reloads the entity
before saving) is tested via the adapter rather than via
PaymentJpaRepository.saveAndFlush.
---
Duplicate comments:
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`:
- Around line 121-123: Replace the weak size-only assertion on the results from
adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED) with an
order-agnostic assertion that the exact rows are returned: assert that the
collection contains the expected payment IDs and that each returned Payment has
PaymentState.INITIATED (e.g., by extracting id and state from results and
comparing as sets or comparing a Set of ids and verifying all states equal
PaymentState.INITIATED), referencing the results variable and the
adapter.findBySenderIdAndState call to locate the code.
- Around line 60-64: The assertion currently ignores all Instant fields in the
round-trip check (the adapter.findById(...).isPresent().get() assertion chain),
which hides DB-level truncation or loss of createdAt/updatedAt/expiresAt; remove
ignoringFieldsOfTypes(Instant.class) and instead assert those Instant fields
explicitly by comparing them at DB precision (e.g., truncate or round both saved
and returned createdAt/updatedAt/expiresAt to the database precision) within the
same recursive comparison or by separate assertions; apply the same change to
the other occurrences mentioned (the similar chains at the other ranges) so each
Instant field is compared deterministically rather than excluded.
- Around line 168-176: The test currently only checks the returned object's
metadata map; instead assert the persisted raw JSONB column is non-null and
equals an empty JSON object: after calling adapter.save(payment) (in
PaymentPersistenceAdapterIT) flush or use the same persistence layer (e.g.,
entityManager, jdbcTemplate, or repository) to query the saved row for
saved.paymentId() and read the metadata column value directly, then assert the
column is not NULL and its string/value equals "{}" (or an empty JSON object) in
addition to the existing mapper assertion. Use the concrete persistence accessor
available in the test (entityManager/ jdbcTemplate/ repository) to fetch the raw
column for verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbad2531-bae4-4232-9e7e-9961cdc21f6a
📒 Files selected for processing (3)
payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/domain/port/PaymentRepository.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapter.java
Comment on lines
+181
to
+205
| @Test | ||
| @DisplayName("should detect optimistic locking conflict on concurrent update") | ||
| void shouldDetectOptimisticLockingConflict() { | ||
| var payment = anInitiatedPayment(); | ||
| adapter.save(payment); | ||
|
|
||
| // Load entity and detach to simulate a separate session | ||
| var entity1 = jpaRepository.findById(payment.paymentId()).orElseThrow(); | ||
| var staleVersion = entity1.getVersion(); | ||
|
|
||
| // First update via adapter increments the version | ||
| var transitioned = payment.startComplianceCheck(); | ||
| adapter.save(transitioned); | ||
|
|
||
| // Build a stale entity with the old version to simulate concurrent access | ||
| var staleEntity = mapper.toEntity(payment); | ||
| staleEntity.setVersion(staleVersion); | ||
| staleEntity.setState(PaymentState.FAILED); | ||
| staleEntity.setUpdatedAt(Instant.now()); | ||
|
|
||
| // Clear persistence context so Hibernate treats staleEntity as detached | ||
| entityManager.clear(); | ||
|
|
||
| assertThatThrownBy(() -> jpaRepository.saveAndFlush(staleEntity)) | ||
| .isInstanceOf(ObjectOptimisticLockingFailureException.class); |
There was a problem hiding this comment.
Exercise optimistic locking through PaymentPersistenceAdapter.
This only proves PaymentJpaRepository honors @Version. Line 27-33 in payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapter.java reloads the current entity before saving, so a stale aggregate can overwrite newer state without surfacing ObjectOptimisticLockingFailureException. Please reproduce the conflict via adapter.save(...); otherwise the lost-update path in the port remains untested.
🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java`
around lines 181 - 205, The test currently exercises JPA directly instead of the
adapter, so update it to reproduce the optimistic-lock conflict through the
adapter: build a stale domain aggregate (use mapper.toEntity or create a domain
Payment with the old version), set its version to staleVersion and a new state
(FAILED), clear the EntityManager, then call
PaymentPersistenceAdapter.save(staleAggregate) (adapter.save(...)) and assert
that it throws ObjectOptimisticLockingFailureException; this ensures the broken
path in PaymentPersistenceAdapter.save (which currently reloads the entity
before saving) is tested via the adapter rather than via
PaymentJpaRepository.saveAndFlush.
…(STA-108) - PaymentPersistenceAdapterIT: 13 tests covering save/retrieve round-trip, findByIdempotencyKey, findBySenderIdAndState, JSONB metadata, optimistic locking conflict, unique constraint violation, and state transition updates - PaymentAuditLogRepositoryIT: 7 tests covering append-only insert, query by payment_id ordered by occurred_at desc, JSONB metadata round-trip - V3 migration: fix version column type INT -> BIGINT (Hibernate maps Long) - TestKafkaConfig: typed KafkaTemplate<String, Object> for outbox handler - AbstractIntegrationTest: @beforeeach database cleanup + entityManager.clear() - application-integration-test.yml: full JPA, Flyway, Kafka, namastack config Closes STA-108 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ndState, full-path test (STA-108) - Add findBySenderIdAndState to PaymentRepository port and implement in adapter so tests go through the adapter layer, not bypass to JPA directly - Rewrite shouldUpdatePaymentThroughFullHappyPath to walk through all state transitions from INITIATED to COMPLETED instead of inserting a pre-built fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace scattered metadata assertThat calls with single usingRecursiveComparison().isEqualTo(expected) for JSONB tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-108-s1-persistence-tests
branch
from
28b6eef to
5544d0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java (1)
121-123:⚠️ Potential issue | 🟡 MinorStrengthen assertions on returned payments.
The test correctly routes through the adapter now, but
hasSize(2)doesn't verify these are the exact payments you saved or that their states areINITIATED. Extract the saved payment IDs and assert the results contain them with the expected state.💡 Proposed improvement
+ var saved1 = adapter.save(payment1); + var saved2 = adapter.save(payment2); - adapter.save(payment1); - adapter.save(payment2); var results = adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED); - assertThat(results).hasSize(2); + assertThat(results) + .hasSize(2) + .extracting(Payment::paymentId) + .containsExactlyInAnyOrder(saved1.paymentId(), saved2.paymentId()); + assertThat(results).allMatch(p -> p.state() == PaymentState.INITIATED);🤖 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/infrastructure/persistence/PaymentPersistenceAdapterIT.java` around lines 121 - 123, The test currently only checks size; update it to assert that the returned payments from adapter.findBySenderIdAndState(SENDER_ID, PaymentState.INITIATED) include the specific saved payment IDs and that each returned payment has state PaymentState.INITIATED. Locate the saved payment fixtures/variables used earlier in the test (e.g., the saved payment objects or their IDs), extract their IDs, and replace the loose hasSize(2) with assertions that results containsExactlyInAnyOrder/their IDs (or contains IDs) and that results.stream().allMatch(p -> p.getState() == PaymentState.INITIATED) to verify states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`:
- Around line 268-307: The test currently saves each domain object but continues
to call subsequent transition methods on the original in-memory objects (e.g.,
payment.startComplianceCheck(), complianceCheck.lockFxRate(...),
fxLocked.startFiatCollection(), etc.), which misses verifying the persisted
round-tripped state; fix this by chaining each transition off the object
returned from adapter.save (assign the result of adapter.save(...) to a variable
and call the next transition on that saved object for every step through
lockFxRate, startFiatCollection, confirmFiatCollected, submitOnChain,
confirmOnChain, initiateOffRamp, settle, complete) so the test exercises state
after persistence before asserting via adapter.findById(...).
- Around line 251-266: The test discards adapter.save() return values causing
state drift; change the flow to chain off saved entities by assigning the
results of adapter.save(...) before calling transitions: call and assign payment
= adapter.save(payment) before payment.startComplianceCheck(), assign
complianceCheck = adapter.save(complianceCheck) before
complianceCheck.lockFxRate(...), and assign fxLocked = adapter.save(fxLocked)
before asserting via adapter.findById(payment.paymentId()); this ensures any
modified fields (versions, timestamps, IDs) returned by save are used for
subsequent transitions and comparisons.
---
Duplicate comments:
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java`:
- Around line 121-123: The test currently only checks size; update it to assert
that the returned payments from adapter.findBySenderIdAndState(SENDER_ID,
PaymentState.INITIATED) include the specific saved payment IDs and that each
returned payment has state PaymentState.INITIATED. Locate the saved payment
fixtures/variables used earlier in the test (e.g., the saved payment objects or
their IDs), extract their IDs, and replace the loose hasSize(2) with assertions
that results containsExactlyInAnyOrder/their IDs (or contains IDs) and that
results.stream().allMatch(p -> p.getState() == PaymentState.INITIATED) to verify
states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 305c0621-b7b7-4c68-9c8f-e60129b1adc2
📒 Files selected for processing (1)
payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/infrastructure/persistence/PaymentPersistenceAdapterIT.java
5 tasks
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
Infrastructure persistence tests for the Payment Orchestrator (S1), covering PaymentPersistenceAdapter and PaymentAuditLogEntity with 20 integration tests using TestContainers PostgreSQL.
Related Issue
Closes STA-108
Type of Change
What Changed
PaymentPersistenceAdapterIT(13 tests) — save/retrieve round-trip, idempotency key lookup, sender+state queries, optimistic locking, JSONB metadata, FX rate persistence, full state transitionPaymentAuditLogRepositoryIT(7 tests) — append-only insert, ordering, filtering, JSONB round-tripAbstractIntegrationTestwith TestContainers PostgreSQL singleton and@BeforeEachdatabase cleanupversioncolumn type from INT to BIGINT (matches Hibernate Long@Version)TestKafkaConfigfor IT context KafkaTemplate bean resolutionHow Was It Tested
Breaking Changes
N/A
Security Considerations
Checklist
./gradlew spotlessCheckpassesCHANGELOG.mdupdated (for features and bug fixes)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores