From 2213848573afbec7672413dd1881b9a53d8dac29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Wokacz?= Date: Thu, 30 Jul 2026 14:47:49 +0200 Subject: [PATCH] fix(spring-config): unwrap DataSource at SpringTransactionContextValidator and use it at isInActiveReadWriteTransaction() --- .../okapi/springboot/DataSourceUnwrapping.kt | 52 +++++++++++++ .../springboot/OutboxAutoConfiguration.kt | 38 ---------- .../SpringTransactionContextValidator.kt | 25 ++++++- .../springboot/DataSourceUnwrappingTest.kt | 74 +++++++++++++++++++ ...xAutoConfigurationTransactionRunnerTest.kt | 59 --------------- .../SpringTransactionContextValidatorTest.kt | 59 +++++++++++++++ 6 files changed, 208 insertions(+), 99 deletions(-) create mode 100644 okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrapping.kt create mode 100644 okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrappingTest.kt diff --git a/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrapping.kt b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrapping.kt new file mode 100644 index 0000000..cac1219 --- /dev/null +++ b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrapping.kt @@ -0,0 +1,52 @@ +package com.softwaremill.okapi.springboot + +import org.springframework.jdbc.datasource.DelegatingDataSource +import java.util.Collections +import java.util.IdentityHashMap +import javax.sql.DataSource + +/** + * Result of walking a [DelegatingDataSource] chain (e.g. a `TransactionAwareDataSourceProxy`) to + * its concrete backing DataSource. + * + * Shared between [OutboxAutoConfiguration] (startup PTM<->DataSource binding check) and + * [SpringTransactionContextValidator] (runtime `publish()` check) so both agree on what "the same + * DataSource" means. Before this was shared (issue #91), the startup check unwrapped + * `DelegatingDataSource` chains before comparing identity but the runtime check compared the raw + * bean reference — a `TransactionAwareDataSourceProxy` (Spring's own documented pattern: proxy as + * the DataSource bean, PlatformTransactionManager on the raw target) passed startup validation and + * then made every `publish()` fail, since the PTM's bound resource lives under the raw target, not + * the proxy. + */ +internal sealed interface Unwrapped { + data class Resolved(val ds: DataSource) : Unwrapped + + /** + * Unwrap stopped before reaching a concrete backing DataSource. Identity comparison would + * be inconclusive — callers must NOT treat this as a mismatch. + */ + data class Unresolvable(val stoppedAt: DataSource, val reason: Reason) : Unwrapped + + enum class Reason { CYCLE, NULL_TARGET } +} + +// Iterative walk with an IdentityHashMap visited-set: guards against cyclic chains +// (Spring's setTargetDataSource has no cycle check). Identity, not equals(), because a +// custom DS overriding equals() to delegate to its target could trigger false early +// termination on a valid chain. +internal fun unwrapDataSource(ds: DataSource): Unwrapped { + val seen: MutableSet = Collections.newSetFromMap(IdentityHashMap()) + var current: DataSource = ds + while (current is DelegatingDataSource) { + if (!seen.add(current)) return Unwrapped.Unresolvable(current, Unwrapped.Reason.CYCLE) + val target = current.targetDataSource + ?: return Unwrapped.Unresolvable(current, Unwrapped.Reason.NULL_TARGET) + current = target + } + return Unwrapped.Resolved(current) +} + +internal fun describeUnwrap(side: String, original: DataSource, unwrapped: Unwrapped): String = when (unwrapped) { + is Unwrapped.Resolved -> "$side side: $original resolved to ${unwrapped.ds}" + is Unwrapped.Unresolvable -> "$side side: $original stopped at ${unwrapped.stoppedAt} (${unwrapped.reason})" +} diff --git a/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfiguration.kt b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfiguration.kt index 0f43017..4c49f1a 100644 --- a/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfiguration.kt +++ b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfiguration.kt @@ -28,13 +28,10 @@ import org.springframework.boot.context.properties.EnableConfigurationProperties import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.core.annotation.Order -import org.springframework.jdbc.datasource.DelegatingDataSource import org.springframework.transaction.PlatformTransactionManager import org.springframework.transaction.support.ResourceTransactionManager import org.springframework.transaction.support.TransactionTemplate import java.time.Clock -import java.util.Collections -import java.util.IdentityHashMap import javax.sql.DataSource /** @@ -456,41 +453,6 @@ class OutboxAutoConfiguration( } } - /** Result of walking a [DelegatingDataSource] chain to its concrete backing DataSource. */ - sealed interface Unwrapped { - data class Resolved(val ds: DataSource) : Unwrapped - - /** - * Unwrap stopped before reaching a concrete backing DataSource. Identity comparison would - * be inconclusive — callers must NOT treat this as a mismatch. - */ - data class Unresolvable(val stoppedAt: DataSource, val reason: Reason) : Unwrapped - - enum class Reason { CYCLE, NULL_TARGET } - } - - // Iterative walk with an IdentityHashMap visited-set: guards against cyclic chains - // (Spring's setTargetDataSource has no cycle check). Identity, not equals(), because a - // custom DS overriding equals() to delegate to its target could trigger false early - // termination on a valid chain. - internal fun unwrapDataSource(ds: DataSource): Unwrapped { - val seen: MutableSet = Collections.newSetFromMap(IdentityHashMap()) - var current: DataSource = ds - while (current is DelegatingDataSource) { - if (!seen.add(current)) return Unwrapped.Unresolvable(current, Unwrapped.Reason.CYCLE) - val target = current.targetDataSource - ?: return Unwrapped.Unresolvable(current, Unwrapped.Reason.NULL_TARGET) - current = target - } - return Unwrapped.Resolved(current) - } - - private fun describeUnwrap(side: String, original: DataSource, unwrapped: Unwrapped): String = when (unwrapped) { - is Unwrapped.Resolved -> "$side side: $original resolved to ${unwrapped.ds}" - is Unwrapped.Unresolvable -> - "$side side: $original stopped at ${unwrapped.stoppedAt} (${unwrapped.reason})" - } - // JPA/Hibernate PTMs that expose a `public DataSource getDataSource()` — reflection by name // avoids requiring spring-orm on the compile classpath (it's optional for JDBC-only consumers). // Hibernate's TM is `org.springframework.orm.jpa.hibernate.HibernateTransactionManager` in diff --git a/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidator.kt b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidator.kt index c781f81..7aa2f89 100644 --- a/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidator.kt +++ b/okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidator.kt @@ -24,11 +24,32 @@ import javax.sql.DataSource * check is the primary DataSource-specific guard. */ internal class SpringTransactionContextValidator( - private val dataSource: DataSource, + dataSource: DataSource, ) : TransactionContextValidator { + + // Spring's own guidance is that a TransactionAwareDataSourceProxy (or any DelegatingDataSource + // wrapper) must NOT be handed to a PlatformTransactionManager -- the PTM is always constructed + // with the raw target, so it binds its transactional resource under that raw reference, never + // under the wrapper. If [dataSource] is such a wrapper, resolving it here to the concrete + // backing DataSource makes getResource() below find what the PTM actually bound, matching what + // OutboxAutoConfiguration's startup PTM<->DataSource check already verifies (issue #91: the two + // checks previously disagreed -- startup unwrapped the chain, this runtime check compared the + // raw bean reference -- so a proxy that passed startup validation then made every publish() + // fail). Resolved once here at construction time, not per-publish(): identity never changes + // after wiring, and this runs on every publish() call. + private val resolvedDataSource: DataSource = when (val unwrapped = unwrapDataSource(dataSource)) { + is Unwrapped.Resolved -> unwrapped.ds + is Unwrapped.Unresolvable -> error( + "Could not resolve the outbox DataSource for transaction validation: " + + describeUnwrap("outbox", dataSource, unwrapped) + + ". Fix the proxy wiring (cycle in setTargetDataSource, or initialise the lazy " + + "proxy's targetDataSource before context refresh).", + ) + } + override fun isInActiveReadWriteTransaction(): Boolean = TransactionSynchronizationManager.isActualTransactionActive() && !TransactionSynchronizationManager.isCurrentTransactionReadOnly() && - TransactionSynchronizationManager.getResource(dataSource) != null + TransactionSynchronizationManager.getResource(resolvedDataSource) != null override val failureMessage: String get() = "No active read-write transaction on the outbox DataSource. " + diff --git a/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrappingTest.kt b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrappingTest.kt new file mode 100644 index 0000000..ed6dcb6 --- /dev/null +++ b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/DataSourceUnwrappingTest.kt @@ -0,0 +1,74 @@ +package com.softwaremill.okapi.springboot + +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.kotest.matchers.types.shouldBeInstanceOf +import io.kotest.matchers.types.shouldBeSameInstanceAs +import org.springframework.jdbc.datasource.LazyConnectionDataSourceProxy +import org.springframework.jdbc.datasource.SimpleDriverDataSource +import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy +import javax.sql.DataSource +import kotlin.time.Duration.Companion.seconds + +/** + * Unit tests for [unwrapDataSource]: nested chains, null targets, and cycles. + * + * Extracted from [OutboxAutoConfiguration] (issue #91) into its own top-level utility, shared with + * [SpringTransactionContextValidator], so the startup PTM<->DataSource check and the runtime + * `publish()` check can no longer independently drift on what "the same DataSource" means. + */ +class DataSourceUnwrappingTest : FunSpec({ + test("returns Resolved with the input itself when not a DelegatingDataSource") { + val raw: DataSource = SimpleDriverDataSource() + val result = unwrapDataSource(raw) + result.shouldBeInstanceOf() + result.ds shouldBeSameInstanceAs raw + } + + test("unwraps a single-level TransactionAwareDataSourceProxy to Resolved(raw)") { + val raw: DataSource = SimpleDriverDataSource() + val proxy: DataSource = TransactionAwareDataSourceProxy(raw) + val result = unwrapDataSource(proxy) + result.shouldBeInstanceOf() + result.ds shouldBeSameInstanceAs raw + } + + test("unwraps a nested chain TADP -> LCDP -> raw down to Resolved(raw)") { + val raw: DataSource = SimpleDriverDataSource() + val nested: DataSource = TransactionAwareDataSourceProxy(LazyConnectionDataSourceProxy(raw)) + val result = unwrapDataSource(nested) + result.shouldBeInstanceOf() + result.ds shouldBeSameInstanceAs raw + } + + test("returns Unresolvable(NULL_TARGET) when a DelegatingDataSource has no targetDataSource") { + // LazyConnectionDataSourceProxy ships with a no-arg constructor that leaves targetDataSource null + // until setTargetDataSource is called. The helper must surface this as an explicit Unresolvable + // outcome so callers do not mistake a not-yet-wired proxy for a mismatched DataSource. + val proxy: DataSource = LazyConnectionDataSourceProxy() + val result = unwrapDataSource(proxy) + result.shouldBeInstanceOf() + result.stoppedAt shouldBeSameInstanceAs proxy + result.reason shouldBe Unwrapped.Reason.NULL_TARGET + } + + test("returns Unresolvable(CYCLE) on a self-referencing DelegatingDataSource").config(timeout = 2.seconds) { + val cyclic = LazyConnectionDataSourceProxy() + cyclic.setTargetDataSource(cyclic) + val result = unwrapDataSource(cyclic) + result.shouldBeInstanceOf() + result.stoppedAt shouldBeSameInstanceAs cyclic + result.reason shouldBe Unwrapped.Reason.CYCLE + } + + test("returns Unresolvable(CYCLE) on a longer cycle (A -> B -> A)").config(timeout = 2.seconds) { + val a = LazyConnectionDataSourceProxy() + val b = LazyConnectionDataSourceProxy() + a.setTargetDataSource(b) + b.setTargetDataSource(a) + val result = unwrapDataSource(a) + result.shouldBeInstanceOf() + result.stoppedAt shouldBeSameInstanceAs a + result.reason shouldBe Unwrapped.Reason.CYCLE + } +}) diff --git a/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfigurationTransactionRunnerTest.kt b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfigurationTransactionRunnerTest.kt index f23b042..351b72b 100644 --- a/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfigurationTransactionRunnerTest.kt +++ b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfigurationTransactionRunnerTest.kt @@ -27,7 +27,6 @@ import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.Primary import org.springframework.context.support.GenericApplicationContext import org.springframework.jdbc.datasource.DataSourceTransactionManager -import org.springframework.jdbc.datasource.LazyConnectionDataSourceProxy import org.springframework.jdbc.datasource.SimpleDriverDataSource import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy import org.springframework.transaction.PlatformTransactionManager @@ -37,7 +36,6 @@ import org.springframework.transaction.support.DefaultTransactionStatus import org.springframework.transaction.support.ResourceTransactionManager import org.springframework.transaction.support.TransactionSynchronizationManager import javax.sql.DataSource -import kotlin.time.Duration.Companion.seconds class OutboxAutoConfigurationTransactionRunnerTest : FunSpec({ @@ -612,63 +610,6 @@ class OutboxAutoConfigurationTransactionRunnerTest : FunSpec({ ctx.getBean(TransactionRunner::class.java).shouldBeInstanceOf() } } - - // Unit tests for the unwrapDataSource helper: nested chains, null targets, and cycles. - context("unwrapDataSource") { - test("returns Resolved with the input itself when not a DelegatingDataSource") { - val raw: DataSource = SimpleDriverDataSource() - val result = OutboxAutoConfiguration.unwrapDataSource(raw) - result.shouldBeInstanceOf() - result.ds shouldBeSameInstanceAs raw - } - - test("unwraps a single-level TransactionAwareDataSourceProxy to Resolved(raw)") { - val raw: DataSource = SimpleDriverDataSource() - val proxy: DataSource = TransactionAwareDataSourceProxy(raw) - val result = OutboxAutoConfiguration.unwrapDataSource(proxy) - result.shouldBeInstanceOf() - result.ds shouldBeSameInstanceAs raw - } - - test("unwraps a nested chain TADP -> LCDP -> raw down to Resolved(raw)") { - val raw: DataSource = SimpleDriverDataSource() - val nested: DataSource = TransactionAwareDataSourceProxy(LazyConnectionDataSourceProxy(raw)) - val result = OutboxAutoConfiguration.unwrapDataSource(nested) - result.shouldBeInstanceOf() - result.ds shouldBeSameInstanceAs raw - } - - test("returns Unresolvable(NULL_TARGET) when a DelegatingDataSource has no targetDataSource") { - // LazyConnectionDataSourceProxy ships with a no-arg constructor that leaves targetDataSource null - // until setTargetDataSource is called. The helper must surface this as an explicit Unresolvable - // outcome so callers do not mistake a not-yet-wired proxy for a mismatched DataSource. - val proxy: DataSource = LazyConnectionDataSourceProxy() - val result = OutboxAutoConfiguration.unwrapDataSource(proxy) - result.shouldBeInstanceOf() - result.stoppedAt shouldBeSameInstanceAs proxy - result.reason shouldBe OutboxAutoConfiguration.Companion.Unwrapped.Reason.NULL_TARGET - } - - test("returns Unresolvable(CYCLE) on a self-referencing DelegatingDataSource").config(timeout = 2.seconds) { - val cyclic = LazyConnectionDataSourceProxy() - cyclic.setTargetDataSource(cyclic) - val result = OutboxAutoConfiguration.unwrapDataSource(cyclic) - result.shouldBeInstanceOf() - result.stoppedAt shouldBeSameInstanceAs cyclic - result.reason shouldBe OutboxAutoConfiguration.Companion.Unwrapped.Reason.CYCLE - } - - test("returns Unresolvable(CYCLE) on a longer cycle (A -> B -> A)").config(timeout = 2.seconds) { - val a = LazyConnectionDataSourceProxy() - val b = LazyConnectionDataSourceProxy() - a.setTargetDataSource(b) - b.setTargetDataSource(a) - val result = OutboxAutoConfiguration.unwrapDataSource(a) - result.shouldBeInstanceOf() - result.stoppedAt shouldBeSameInstanceAs a - result.reason shouldBe OutboxAutoConfiguration.Companion.Unwrapped.Reason.CYCLE - } - } }) // Mimics JpaTransactionManager / HibernateTransactionManager: implements ResourceTransactionManager diff --git a/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidatorTest.kt b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidatorTest.kt index 3fd2468..157415d 100644 --- a/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidatorTest.kt +++ b/okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/SpringTransactionContextValidatorTest.kt @@ -1,8 +1,12 @@ package com.softwaremill.okapi.springboot +import io.kotest.assertions.throwables.shouldThrow import io.kotest.core.spec.style.BehaviorSpec import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain +import org.springframework.jdbc.datasource.LazyConnectionDataSourceProxy import org.springframework.jdbc.datasource.SimpleDriverDataSource +import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy import org.springframework.transaction.support.TransactionSynchronizationManager import javax.sql.DataSource @@ -95,4 +99,59 @@ class SpringTransactionContextValidatorTest : BehaviorSpec({ } } } + + // The outbox DataSource bean is a TransactionAwareDataSourceProxy (Spring's own + // documented pattern -- "TransactionAwareDataSourceProxy should NOT be passed to a PTM"), so + // the PlatformTransactionManager binds its resource under the RAW target, never the proxy. + // Before this fix, the validator compared against the raw `dataSource` constructor argument + // (the proxy here) with no unwrapping, so getResource(proxy) always returned null and + // publish() failed on every call despite OutboxAutoConfiguration's startup check having + // already verified this exact wiring as correct. + given("outbox DataSource bean is a TransactionAwareDataSourceProxy wrapping the PTM's raw target") { + then("an active RW transaction with the resource bound to the RAW target is recognised") { + val rawDs: DataSource = SimpleDriverDataSource() + val proxyDs: DataSource = TransactionAwareDataSourceProxy(rawDs) + val proxyValidator = SpringTransactionContextValidator(proxyDs) + + TransactionSynchronizationManager.initSynchronization() + TransactionSynchronizationManager.setActualTransactionActive(true) + TransactionSynchronizationManager.bindResource(rawDs, Any()) + try { + proxyValidator.isInActiveReadWriteTransaction() shouldBe true + } finally { + TransactionSynchronizationManager.unbindResource(rawDs) + TransactionSynchronizationManager.clearSynchronization() + TransactionSynchronizationManager.setActualTransactionActive(false) + } + } + + then("a nested chain (TADP -> LazyConnectionDataSourceProxy -> raw) also resolves to the raw target") { + val rawDs: DataSource = SimpleDriverDataSource() + val nested: DataSource = TransactionAwareDataSourceProxy(LazyConnectionDataSourceProxy(rawDs)) + val nestedValidator = SpringTransactionContextValidator(nested) + + TransactionSynchronizationManager.initSynchronization() + TransactionSynchronizationManager.setActualTransactionActive(true) + TransactionSynchronizationManager.bindResource(rawDs, Any()) + try { + nestedValidator.isInActiveReadWriteTransaction() shouldBe true + } finally { + TransactionSynchronizationManager.unbindResource(rawDs) + TransactionSynchronizationManager.clearSynchronization() + TransactionSynchronizationManager.setActualTransactionActive(false) + } + } + } + + given("outbox DataSource bean is an unresolvable DelegatingDataSource chain") { + then("construction fails fast with an actionable message, instead of every later publish() silently failing validation") { + val cyclic = LazyConnectionDataSourceProxy() + cyclic.setTargetDataSource(cyclic) + + val exception = shouldThrow { + SpringTransactionContextValidator(cyclic) + } + exception.message shouldContain "Could not resolve the outbox DataSource" + } + } })