feat: require TransactionRunner in okapi-spring-boot autoconfig (KOJAK-67)#49
Open
endrju19 wants to merge 8 commits into
Open
feat: require TransactionRunner in okapi-spring-boot autoconfig (KOJAK-67)#49endrju19 wants to merge 8 commits into
endrju19 wants to merge 8 commits into
Conversation
endrju19
added a commit
that referenced
this pull request
May 19, 2026
…-67) Second comprehensive multi-agent review of PR #49. One correctness defect, three documentation/comment fixes, two test-quality additions, and DRY cleanup. Correctness (#1): - validatePtmDataSourceMatch: when a DelegatingDataSource chain terminates early (cycle / uninitialised LazyConnectionDataSourceProxy), the `!==` is inconclusive — we never reached the concrete backing DataSources. Previously logged a WARN then unconditionally threw the generic "bound to a different DataSource" error, asserting an unverified mismatch and misdirecting the operator to qualifier config instead of proxy wiring. Now throws a distinct proxy-chain-unresolvable error; the mismatch error is reserved for the case where both sides fully unwrap to concrete, distinct DataSources. Comments (#3,#4,#8,#9): - Removed duplicate Hibernate package-move paragraph (ktlint-fix commit introduced it); trimmed "Strategy N" labels duplicating the method list; fixed self-referential stale comment ("above, around line 254" was AT 254); corrected Hibernate version comment drift (said 7.0, pin is 7.1.4). Tests (#2,#6,#7): - PROBE test empirically establishes Spring's InitializingBean contract rejects a TransactionTemplate bean with null transactionManager — the factory's `?:` fallback is unreachable-via-Spring defensive code, not an untested gap (production comment updated to point at the proof). - Restored JpaPtmClassesReflectionGuardTest in okapi-integration-tests (spring-orm available there); JPA_HIBERNATE_PTM_CLASSES made public for cross-module access. Guards the set against full bit-rot. - Trimmed deleted-version history from TransactionTemplateHijackProofTest and SpringObjectProviderSemanticsAssumptionsTest KDocs. DRY (#5,#10): - Extracted byte-identical stubStore()/stubDeliverer()/stubDelivererWithType() from 7 test files into OkapiSpringTestStubs.kt; added a reified GenericApplicationContext.registerBean() helper collapsing the BeanDefinitionBuilder.genericBeanDefinition(...).apply{isPrimary} boilerplate (9 call sites). Verified: full ktlintCheck clean, :okapi-spring-boot:test + :okapi-integration-tests:test green, Spring Boot 3.5.12 matrix simulation (-PspringBootVersion=3.5.12 -PspringVersion=6.2.7) green.
endrju19
added a commit
that referenced
this pull request
May 20, 2026
…xes (KOJAK-67)
- Issue 13: correct KDoc grouping — JpaTransactionManager IS-A ResourceTransactionManager
(the non-RTM example is Exposed SpringTransactionManager only)
- Issue 11: comment on okapiTransactionRunner deliberate interface return (required for
@ConditionalOnMissingBean to suppress on ANY user TransactionRunner impl)
- Issue 10: move pitest plugin + tool versions to gradle/libs.versions.toml
(was inline in build.gradle.kts, violating .ai/coding.md)
- Issue 4 : extractDataSource walks the superclass chain, recognising user subclasses
of JpaTransactionManager / HibernateTransactionManager
- Issue 6+12: refactor unwrapDataSource to sealed Unwrapped { Resolved | Unresolvable }
with IdentityHashMap-backed cycle detection; validatePtmDataSourceMatch
now branches on three explicit cases, removing the load-bearing comment
- Issue 5 : fail-fast when non-extractable PTM + >=2 DataSource beans + no qualifier
(single-DS stays on the existing INFO/WARN path). Replace the flaky
amplification-proof integration test with a deterministic startup-failure
test (WrongPtmMultiDataSourceFailFastTest)
- Issue 7+8: rebuild TransactionTemplateHijackProofTest into two clean contract tests
(ADOPTS Boot's TT verbatim + VALIDATES still runs on wrong-DS PTM) and
revert SpringTransactionRunner.transactionTemplate from public to internal
- Issue 9 : add CHANGELOG BREAKING entries for the constructor change and the new
multi-DS fail-fast guard; expand README with multi-DS guidance and a
direct-constructor migration note (linking #49, not the JIRA key)
6 tasks
…K-67) Eliminate the silent JDBC auto-commit fallback that caused duplicate delivery across processor instances when no PlatformTransactionManager was on the classpath. The autoconfig now produces a non-null TransactionRunner via a fail-fast bean factory, or skips it entirely when both scheduler and purger are disabled (publish-only deployments). Production changes (okapi-spring-boot): - new @bean okapiTransactionRunner — derives SpringTransactionRunner from PlatformTransactionManager (qualifier > getIfUnique > distinct error), gated by @ConditionalOnExpression on schedulers being enabled - new okapi.transaction-manager-qualifier property (mirror of okapi.datasource-qualifier) - validatePtmDataSourceMatch — fails fast for ResourceTransactionManager PTMs bound to a different DataSource; logs a WARN for PTMs that don't expose a JDBC DataSource (JPA's EntityManagerFactory, Hibernate's SessionFactory, Exposed bridge, JTA); falls through gracefully when proxy chains terminate early (cycle, null target) with a distinct WARN - unwrapDataSource — iterative with visited-set so a self-referencing DelegatingDataSource doesn't spin the startup thread - OutboxProcessorScheduler / OutboxPurgerScheduler — non-null TransactionRunner parameter (was nullable TransactionTemplate) - spring-configuration-metadata.json entry for the new property Tests: - OutboxAutoConfigurationTransactionRunnerTest — 13 slice tests + 6 unit tests for unwrapDataSource (cycle / null target / nested chain / TADP unwrap / etc.) - SpringObjectProviderSemanticsAssumptionsTest — pins Spring 7 ObjectProvider.getIfUnique / ResourceTransactionManager semantics so framework upgrades surface loudly - ExposedSpringBridgeEndToEndTest — Postgres testcontainer + Exposed SpringTransactionManager bridge, including a multi-instance amplification test that detects regression to the auto-commit fallback - existing tests updated to register a no-op TransactionRunner where schedulers ran without a PTM (Liquibase / scheduler / qualifier tests) Build: - okapi-spring-boot: opt-in PIT mutation testing via -PenableMutationTesting=true - okapi-integration-tests: spring7-transaction + assertj + spring-boot-test for the Exposed bridge end-to-end coverage README: short migration note in the okapi-spring-boot section.
…qualifier safety
Address review-round findings on top of the KOJAK-67 baseline:
- okapiTransactionRunner factory now reads ObjectProvider<TransactionTemplate>
and reuses a unique user-supplied bean instead of silently building a fresh
TransactionTemplate(ptm). User intent (timeout, propagation, isolation)
flows into scheduler ticks.
- resolvePlatformTransactionManager catches BeanNotOfRequiredTypeException
(typo into a DataSource bean name) and rewraps with an okapi-specific
message instead of leaking Spring's generic error.
- validatePtmDataSourceMatch now reports the actual resourceFactory class
in the WARN message (covers custom RTM implementations) and emits an INFO
breadcrumb in the single-DataSource case (no qualifier set, non-RTM PTM)
so future multi-DS migrations have a grep target.
Tests:
- 4 new slice tests in OutboxAutoConfigurationTransactionRunnerTest covering:
* property binding: okapi.transaction-manager-qualifier kebab → camelCase
* blank-string validation via Spring Binder
* BeanNotOfRequiredTypeException → actionable error
* user @bean TransactionTemplate honoured (reference identity check)
* actual resourceFactory class surfaced in WARN
* INFO breadcrumb in single-DS non-RTM scenario
- ExposedSpringBridgeEndToEndTest gains a purger-tick assertion under the
Exposed bridge PTM (DELIVERED rows past retention are actually removed).
- Existing non-RTM WARN test extended with ListAppender-based content
assertions (does not implement RTM / okapi.transaction-manager-qualifier
mention) so the diagnostic cannot rot silently.
Production code adjustments:
- SpringTransactionRunner.transactionTemplate is now internal val so tests
can assert reference identity with the user-supplied template.
- validatePtmDataSourceMatch computes resourceFactoryDescription once and
reuses it across the WARN (multi-DS hint) and INFO (single-DS hint)
branches.
…and always validate PTM↔DS binding (KOJAK-67) Closes a silent-failure path discovered by Spring Boot Test verification: TransactionAutoConfiguration auto-creates a TransactionTemplate bean whenever a single PlatformTransactionManager exists. The previous factory took that TT via ObjectProvider.getIfUnique() and short-circuited PTM resolution + validatePtmDataSourceMatch — every Spring Boot user with multi-DS wiring silently bypassed the safety net. Production logic: - extractDataSource() reflects JpaTransactionManager.getDataSource() and HibernateTransactionManager.getDataSource() (both Spring 6.1- and 6.2+ packages) — narrow NoSuchMethodException catch, all other exceptions intentionally propagate - describeUnextractable() distinct diagnostic per PTM family - okapiTransactionRunner always runs validatePtmDataSourceMatch on the PTM it extracts from the TT (user OR Boot autoconfig), then wraps either the user/Boot TT (preserves timeout/propagation) or a freshly built one - SpringTransactionRunner.transactionTemplate → public (testable API) Tests: - TransactionTemplateHijackProofTest: 3 reliability invariants (PRECONDITION Boot creates TT, INTROSPECTION factory wraps OUR PTM, MISMATCH FAIL-FAST validation runs) — guards regression of the hijack bug - JpaTransactionManagerFailFastTest / MatchedDataSourceTest: real JPA setup proves extractDataSource JPA branch fails fast on mismatch and accepts match - WrongPtmDataSourceAmplificationProofTest: inverted assertion documents residual amplification risk for non-extractable PTMs (JTA, Exposed bridge) - PostgresTestSupport: extracted runOkapiLiquibaseOn() + pgDataSourceOf() helpers (DRY across 3 multi-container tests) Build: - testImpl: spring-orm + hibernate-core (JPA tests), spring-boot-transaction (TransactionAutoConfiguration on Spring Boot 4.0 test classpath)
…= 4 (KOJAK-67) spring-boot-transaction is a Spring Boot 4.0+ artifact (3.x bundles TransactionAutoConfiguration inside spring-boot-autoconfigure). The CI matrix override -PspringBootVersion=3.5.x rewrites every org.springframework.boot:* coordinate via resolutionStrategy, so the unconditional dependency tried to resolve a non-existent spring-boot-transaction:3.5.12 and failed the "Spring Boot 3.5.12" job. TransactionTemplateHijackProofTest already resolves whichever FQCN is present (3.x autoconfigure package OR 4.x dedicated module) via Class.forName fallback, so on 3.x it finds TransactionAutoConfiguration in spring-boot-autoconfigure without the extra dependency. Verified locally: -PspringBootVersion=3.5.12 -PspringVersion=6.2.7 → all 3 hijack tests PASS, no resolution error.
… (KOJAK-67) The multiline `springBootMajorForTests` expression failed ktlintKotlinScriptCheck (build.gradle.kts:43 unexpected indentation). That task is separate from ktlintCheck-over-source-sets and was not run locally after the previous fix. Applied ktlintFormat; full `./gradlew ktlintCheck` now clean across all modules and Kotlin scripts.
…-67) Second comprehensive multi-agent review of PR #49. One correctness defect, three documentation/comment fixes, two test-quality additions, and DRY cleanup. Correctness (#1): - validatePtmDataSourceMatch: when a DelegatingDataSource chain terminates early (cycle / uninitialised LazyConnectionDataSourceProxy), the `!==` is inconclusive — we never reached the concrete backing DataSources. Previously logged a WARN then unconditionally threw the generic "bound to a different DataSource" error, asserting an unverified mismatch and misdirecting the operator to qualifier config instead of proxy wiring. Now throws a distinct proxy-chain-unresolvable error; the mismatch error is reserved for the case where both sides fully unwrap to concrete, distinct DataSources. Comments (#3,#4,#8,#9): - Removed duplicate Hibernate package-move paragraph (ktlint-fix commit introduced it); trimmed "Strategy N" labels duplicating the method list; fixed self-referential stale comment ("above, around line 254" was AT 254); corrected Hibernate version comment drift (said 7.0, pin is 7.1.4). Tests (#2,#6,#7): - PROBE test empirically establishes Spring's InitializingBean contract rejects a TransactionTemplate bean with null transactionManager — the factory's `?:` fallback is unreachable-via-Spring defensive code, not an untested gap (production comment updated to point at the proof). - Restored JpaPtmClassesReflectionGuardTest in okapi-integration-tests (spring-orm available there); JPA_HIBERNATE_PTM_CLASSES made public for cross-module access. Guards the set against full bit-rot. - Trimmed deleted-version history from TransactionTemplateHijackProofTest and SpringObjectProviderSemanticsAssumptionsTest KDocs. DRY (#5,#10): - Extracted byte-identical stubStore()/stubDeliverer()/stubDelivererWithType() from 7 test files into OkapiSpringTestStubs.kt; added a reified GenericApplicationContext.registerBean() helper collapsing the BeanDefinitionBuilder.genericBeanDefinition(...).apply{isPrimary} boilerplate (9 call sites). Verified: full ktlintCheck clean, :okapi-spring-boot:test + :okapi-integration-tests:test green, Spring Boot 3.5.12 matrix simulation (-PspringBootVersion=3.5.12 -PspringVersion=6.2.7) green.
…xes (KOJAK-67)
- Issue 13: correct KDoc grouping — JpaTransactionManager IS-A ResourceTransactionManager
(the non-RTM example is Exposed SpringTransactionManager only)
- Issue 11: comment on okapiTransactionRunner deliberate interface return (required for
@ConditionalOnMissingBean to suppress on ANY user TransactionRunner impl)
- Issue 10: move pitest plugin + tool versions to gradle/libs.versions.toml
(was inline in build.gradle.kts, violating .ai/coding.md)
- Issue 4 : extractDataSource walks the superclass chain, recognising user subclasses
of JpaTransactionManager / HibernateTransactionManager
- Issue 6+12: refactor unwrapDataSource to sealed Unwrapped { Resolved | Unresolvable }
with IdentityHashMap-backed cycle detection; validatePtmDataSourceMatch
now branches on three explicit cases, removing the load-bearing comment
- Issue 5 : fail-fast when non-extractable PTM + >=2 DataSource beans + no qualifier
(single-DS stays on the existing INFO/WARN path). Replace the flaky
amplification-proof integration test with a deterministic startup-failure
test (WrongPtmMultiDataSourceFailFastTest)
- Issue 7+8: rebuild TransactionTemplateHijackProofTest into two clean contract tests
(ADOPTS Boot's TT verbatim + VALIDATES still runs on wrong-DS PTM) and
revert SpringTransactionRunner.transactionTemplate from public to internal
- Issue 9 : add CHANGELOG BREAKING entries for the constructor change and the new
multi-DS fail-fast guard; expand README with multi-DS guidance and a
direct-constructor migration note (linking #49, not the JIRA key)
…ments (KOJAK-67) - Multi-DS ambiguity guard now requires `okapi.transaction-manager-qualifier` to be set when the PTM is non-extractable AND >=2 DataSource beans exist; setting only `okapi.datasource-qualifier` is no longer sufficient (it picks the outbox DS but does not constrain which PTM brackets it — PTM-side ambiguity remains). New test in WrongPtmMultiDataSourceFailFastTest pins the strictness; the previous escape-hatch test still passes. - README and CHANGELOG updated to reflect the tighter contract. - Comment audit across the changed files: removed redundant comments that just restated code/annotations or referenced the deleted amplification-proof test; shortened verbose rationale blocks; preserved load-bearing WHY/gotcha context (Spring proxy idioms, IdentityHashMap rationale, JPA package-rename history, no-Class.forName constraint). Net: ~86 lines of comments removed across 11 files.
5ffff58 to
3e9a785
Compare
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
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
Closes the silent JDBC auto-commit fallback in
OutboxAutoConfigurationthat allowed duplicate delivery across processor instances when noPlatformTransactionManagerwas on the classpath. Replaces it with a fail-fast bean factory plus a structured safety net for the multi-DataSource / multi-PTM / proxy edge cases discovered during review.Why this matters
Under the prior code path, scheduler ticks ran in JDBC auto-commit when no PTM was registered.
FOR UPDATE SKIP LOCKEDreleased its row lock as soon as the SELECT statement returned — beforeupdateAfterProcessingfinished — so a second processor instance could claim the same entry and deliver it again. Zero error, zero log, invisible until horizontal scaling.What changed
Production (
okapi-spring-boot):@Bean okapiTransactionRunnerfactory — derivesSpringTransactionRunnerfromPlatformTransactionManager(qualifier > getIfUnique > distinct error); honours a user-supplied@Bean TransactionTemplatewhen present; gated by@ConditionalOnExpressionso publish-only deployments (both schedulers disabled) start without requiring a PTM. Returns theTransactionRunnerinterface deliberately so a user-supplied@Bean TransactionRunnerof any impl suppresses the factory via@ConditionalOnMissingBean(documented inline).okapi.transaction-manager-qualifierproperty (mirror ofokapi.datasource-qualifier), includingspring-configuration-metadata.jsonentry for IDE autocomplete.validatePtmDataSourceMatch— fails fast when an RTM-based PTM is bound to a differentDataSourcethan okapi's outbox; also fails fast when the PTM is non-extractable AND ≥2DataSourcebeans exist AND neither qualifier is set (multi-DS ambiguity guard); logs an actionable WARN for non-extractable PTMs in single-DS or qualified setups; INFO breadcrumb in single-DS contexts; rewrapsBeanNotOfRequiredTypeExceptionwith okapi-specific context.extractDataSourcewalks the superclass chain, recognising user subclasses ofJpaTransactionManager/HibernateTransactionManager.unwrapDataSourcereturns a sealedUnwrapped { Resolved | Unresolvable(reason: CYCLE | NULL_TARGET) }instead of an overloadedDataSource;validatePtmDataSourceMatchbranches on three explicit cases. Cycle detection usesIdentityHashMap(reference identity), defending against customDataSourceimpls whoseequals()delegates to their target.OutboxProcessorScheduler/OutboxPurgerSchedulerconstructors take a non-nullTransactionRunner(was nullableTransactionTemplate).SpringTransactionRunner.transactionTemplatestaysinternal— the field is visible to same-module tests only, not part of the published API.Tests:
OutboxAutoConfigurationTransactionRunnerTest— slice + unit tests covering multi-PTM, qualifier resolution, RTM/non-RTM/JPA-RTM validation, property binding, publish-only mode, user-template honoured, andUnwrappedcases (Resolved / Unresolvable.CYCLE / Unresolvable.NULL_TARGET).TransactionTemplateHijackProofTest— two clean contract tests: ADOPTS Boot's auto-TransactionTemplateverbatim (shouldBeSameInstanceAs), VALIDATES the multi-DS safety net is not bypassed.SpringObjectProviderSemanticsAssumptionsTest— pins Spring 7ObjectProvider.getIfUnique/ResourceTransactionManagercontract so framework upgrades surface loudly.ExposedSpringBridgeEndToEndTest— Postgres testcontainer + ExposedSpringTransactionManagerbridge, including a multi-instance amplification test and a purger E2E proving the bridged PTM works end-to-end.JpaTransactionManagerFailFastTest/JpaTransactionManagerMatchedDataSourceTest/JpaPtmClassesReflectionGuardTest— fail-fast and match paths for the JPA branch + bit-rot guard onJPA_HIBERNATE_PTM_CLASSES.WrongPtmMultiDataSourceFailFastTest— deterministic startup-failure assertion for the multi-DS ambiguity guard, plus a companion test provingokapi.transaction-manager-qualifieris the escape hatch. Replaces the earlier flakyWrongPtmDataSourceAmplificationProofTest(race-dependent positive assertion, semantically inverted).Build / infrastructure:
okapi-spring-boot: opt-in PIT mutation testing via-PenableMutationTesting=true. Plugin and tool versions live ingradle/libs.versions.toml(project convention).okapi-integration-tests:spring7-transaction+assertj+spring-boot-testfor the Exposed bridge end-to-end coverage.Docs:
CHANGELOG.md[Unreleased] ### Changed (BREAKING)entries for the constructor change and the new multi-DS ambiguity fail-fast.TransactionRunnerrequirement for autoconfig users, (b) multi-DataSource disambiguation, (c) the direct-constructor path for non-autoconfig consumers.Test plan
./gradlew checkpasses (Postgres + MySQL + Kafka testcontainers, all modules)ExposedSpringBridgeEndToEndTest's mutation-verified concurrent processors testokapi.transaction-manager-qualifierYAML contractokapi_databasechangelogtables to isolate library migration state #37) still pass post-rebaseOut of scope (separate follow-up tickets)
These pre-existing concerns surfaced during review but are NOT part of this PR's scope — they live in
okapi-coreor predate this change:OutboxScheduler/OutboxPurgerstill accept a nullableTransactionRunner? = nulland fall back to non-transactional execution — Ktor / direct-core users can still hit the original auto-commit bug. Tracked in core: require non-null TransactionRunner in OutboxScheduler / OutboxPurger (breaking) #51 (breaking, no escape hatch).OutboxScheduler.tick()/OutboxPurger.tick()swallow-and-retry-forever — log+retry on every exception with no escalation or listener callback for consecutive failures.OutboxStatus.from()throws on a corrupt status string, halting the entire pipeline.SpringTransactionRunner.runInTransactionuses!!— NPE risk for nullable generic types (currently unreachable, future-fragile).Each of the above is a self-contained problem worth its own ticket and dedicated test coverage.