fix(repos): catch Spring DataAccessException so CrdbRetry and PROCTRAN abends fire#37
fix(repos): catch Spring DataAccessException so CrdbRetry and PROCTRAN abends fire#37
Conversation
…N abends fire Spring Boot 3's JooqAutoConfiguration registers DefaultExceptionTranslatorExecuteListener, which intercepts every SQLException raised by jOOQ and substitutes an org.springframework.dao.DataAccessException via ctx.exception(...). The Spring DAE is not a subtype of org.jooq.exception.DataAccessException, so every catch (org.jooq.exception.DataAccessException) block in this codebase missed at runtime. Concretely: - CrdbRetry.run(...) did not retry SQLSTATE 40001 — the Spring DAE bypassed the catch and propagated immediately. - Inner PROCTRAN catches (CrecustRepository, UpdcustRepository, CreaccRepository, UpdaccRepository, DbcrfunRepository) did not translate failures into HWPT or domain fail codes. - Outer XRTY catches added in #33 did not fire either. Swap the import to org.springframework.dao.DataAccessException across all 13 main files. The catch sites use the simple class name only, so the body is unchanged. Test mocks already used 'new DataAccessException(...) { }' anonymous subclasses (Spring DAE is abstract; jOOQ DAE is concrete but the anonymous form works for both), so unit tests need only the import swap as well. Add the regression integration tests deferred from #34: - CrecustServiceIntegrationTest#proctranInsertFailureSurfacesHwptAbend - UpdcustServiceIntegrationTest#proctranInsertFailureSurfacesHwptAbend Both force every PROCTRAN insert to fail via 'CHECK (false) NOT VALID' and assert the HWPT abend surfaces, exercising the inner catches that unit tests cannot reach (the lambda runs against DSL.using(configuration) inside transactionResult). Document the constraint in translation-rules.md alongside the existing PROCTRAN audit-trail rule so future code generation imports the right DAE. Closes #36
|
augment review |
![augment-app-staging[bot]](https://avatars.githubusercontent.com/in/1008152?s=60&v=4){kind=small}
| // never matches and the test sees a Spring DataIntegrityViolation | ||
| // bubble out instead, classified as UNEX. | ||
| when(reviewDateRandom.nextInt(20)).thenReturn(3); | ||
| dsl.execute("ALTER TABLE proctran ADD CONSTRAINT proctran_block_inserts CHECK (false) NOT VALID"); |
There was a problem hiding this comment.
Because AbstractCockroachIntegrationTest shares a singleton Cockroach container across test classes, if this ALTER TABLE ... ADD CONSTRAINT throws (e.g. constraint already exists from a prior failure or parallel execution), the test will exit before the finally and leave the schema poisoned for later tests. Consider ensuring the schema cleanup runs even when the constraint-add step fails.
Severity: medium
Other Locations
src/test/java/com/augment/cbsa/service/UpdcustServiceIntegrationTest.java:112
🤖 Was this useful? React with 👍 or 👎
| // never matches and the test sees a Spring DataIntegrityViolation | ||
| // bubble out instead, classified as UNEX. | ||
| when(reviewDateRandom.nextInt(20)).thenReturn(3); | ||
| dsl.execute("ALTER TABLE proctran ADD CONSTRAINT proctran_block_inserts CHECK (false) NOT VALID"); |
There was a problem hiding this comment.
Because the ALTER TABLE ... ADD CONSTRAINT runs outside the try/finally, if it fails (e.g., the constraint already exists from a previous failed run), the cleanup won’t execute and later integration tests may be affected by the leftover constraint.
Severity: medium
Other Locations
src/test/java/com/augment/cbsa/service/UpdcustServiceIntegrationTest.java:112
🤖 Was this useful? React with 👍 or 👎
Round-1 review noted that the ALTER TABLE ... ADD CONSTRAINT call sat outside the try/finally, so a failure on the ADD itself (e.g. the constraint already exists from a previously-killed JVM, since AbstractCockroachIntegrationTest reuses a singleton container across test classes) would skip the cleanup and poison every later PROCTRAN write in the same JVM. Move the ADD inside the try and use DROP CONSTRAINT IF EXISTS for the cleanup so the schema returns to a known-good state regardless of where the test fails.
Round 1 → Round 2 responseBoth review rounds flagged the same medium issue on the integration tests: the Fixed in d38fbc9 by:
Local verify still green (7/7 across the two integration test classes). augment review |
| // use DROP ... IF EXISTS so we also recover from any constraint a | ||
| // previously-killed JVM may have left behind. | ||
| try { | ||
| dsl.execute("ALTER TABLE proctran ADD CONSTRAINT proctran_block_inserts CHECK (false) NOT VALID"); |
There was a problem hiding this comment.
If proctran_block_inserts already exists before this test starts (e.g., from a previously-killed JVM in a shared Cockroach container), the ALTER TABLE ... ADD CONSTRAINT will throw before the assertion runs and make the test flaky. The comment mentions recovering from leftover constraints, but that recovery currently only happens in the finally after the add attempt.
Severity: medium
Other Locations
src/test/java/com/augment/cbsa/service/UpdcustServiceIntegrationTest.java:119
🤖 Was this useful? React with 👍 or 👎
Round-2 review: even with the cleanup moved into try/finally, a leftover proctran_block_inserts constraint from a previously-killed JVM would still make the next test flaky because the recovery only ran in finally, after the ADD. Drop pre-emptively before adding so the test is idempotent on both ends.
Round 2 → Round 3 responseRound 2 narrowed the medium issue to the on-the-way-in case: a leftover constraint from a previously-killed JVM would still make the next test flaky because the recovery only ran in Fixed by adding a pre-emptive
Local verify still green (7/7 across the two integration test classes). augment review |
1 participant
Problem
Per #36: Spring Boot 3's
JooqAutoConfigurationregistersDefaultExceptionTranslatorExecuteListener, which intercepts everySQLExceptionraised by jOOQ and substitutes anorg.springframework.dao.DataAccessExceptionviactx.exception(...). TheSpring DAE is not a subtype of
org.jooq.exception.DataAccessException,so every
catch (org.jooq.exception.DataAccessException)block in thiscodebase missed at runtime. Concretely:
CrdbRetry.run(...)did not retrySQLSTATE 40001— the Spring DAEbypassed the catch and propagated immediately.
CrecustRepository,UpdcustRepository,CreaccRepository,UpdaccRepository,DbcrfunRepository) did nottranslate failures into
HWPTor domain fail codes.XRTYcatches added in fix(proctran): standardise audit-trail write failures on CbsaAbendException(HWPT) #33 did not fire either.The global
@ControllerAdvicestill classified the uncaught Spring DAEas
UNEX, which is why no test alarm went off until now: the only way toobserve the gap is to make a write actually fail at the database layer,
which the existing test suite did not exercise.
Fix
org.springframework.dao.DataAccessExceptionacrossall 13 main files. The catch sites use the simple class name only, so
the bodies are unchanged.
isSerializationFailure(...)already walksthe cause chain looking for
SQLExceptionwithSQLSTATE = 40001, whichworks the same way against either DAE flavour.
CbsaAbendExceptionaccepts any
Throwableas cause, so no further adjustments are needed.new DataAccessException(...) { }anonymoussubclasses (Spring DAE is abstract; jOOQ DAE is concrete but the
anonymous form works for both), so unit tests need only the import
swap as well.
Regression coverage
Add the integration tests deferred from #34:
CrecustServiceIntegrationTest#proctranInsertFailureSurfacesHwptAbendUpdcustServiceIntegrationTest#proctranInsertFailureSurfacesHwptAbendBoth force every PROCTRAN insert to fail via
ALTER TABLE proctran ADD CONSTRAINT ... CHECK (false) NOT VALIDandassert that
CbsaAbendException("HWPT")surfaces, exercising the innercatches that unit tests cannot reach (the lambda runs against
DSL.using(configuration)insidetransactionResult).Pre-fix, both tests fail with
org.springframework.dao.DataIntegrityViolationExceptionbubbling outof the service. Post-fix, both pass.
Docs
Add a bullet to
docs/translation-rules.md§12 spelling out which DAE toimport and why, so future code generation does not regress.
Verification
./mvnw -B verify→ 187 tests pass, including the two newintegration tests.
the Spring DAE that the translator actually emits.
Closes #36.