Skip to content

Commit a8fb913

Browse files
endrju19endrju19
committed
refactor(spring-boot): apply full PR re-review findings #1-#10 (KOJAK-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.
1 parent 4a7ee96 commit a8fb913

12 files changed

Lines changed: 160 additions & 209 deletions

File tree

‎gradle/libs.versions.toml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ h2 = "2.4.240"
2020
micrometer = "1.16.5"
2121
jmh = "1.37"
2222
jmhGradlePlugin = "0.7.3"
23-
# Hibernate version aligned with what Spring 7.x targets (Hibernate 7.0 ORM); only used in the
24-
# integration-tests module to exercise JpaTransactionManager fail-fast extraction.
23+
# Hibernate ORM 7.x (the line Spring Framework 7.x targets); integration-tests only, to exercise
24+
# JpaTransactionManager fail-fast extraction.
2525
hibernate = "7.1.4.Final"
2626

2727
[libraries]

‎okapi-integration-tests/src/test/kotlin/com/softwaremill/okapi/test/transaction/JpaPtmClassesReflectionGuardTest.kt‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
package com.softwaremill.okapi.test.transaction
2+
3+
import com.softwaremill.okapi.springboot.OutboxAutoConfiguration
4+
import io.kotest.assertions.withClue
5+
import io.kotest.core.spec.style.FunSpec
6+
import io.kotest.matchers.collections.shouldNotBeEmpty
7+
8+
/**
9+
* Guards `OutboxAutoConfiguration.JPA_HIBERNATE_PTM_CLASSES` against full bit-rot: if all listed
10+
* FQCNs become unresolvable (Spring rename / package move), `extractDataSource`'s JPA branch
11+
* silently never fires and every JPA user falls through to the WARN-only path. Runs in
12+
* okapi-integration-tests because spring-orm is a `testImplementation` there. Mirrors the
13+
* `@AutoConfigureAfter`-resolves guard on OkapiMicrometerAutoConfiguration.
14+
*/
15+
class JpaPtmClassesReflectionGuardTest : FunSpec({
16+
17+
test("at least one FQCN in JPA_HIBERNATE_PTM_CLASSES resolves on the runtime classpath") {
18+
val classLoader = OutboxAutoConfiguration::class.java.classLoader
19+
val resolvable = OutboxAutoConfiguration.JPA_HIBERNATE_PTM_CLASSES.filter { fqcn ->
20+
try {
21+
Class.forName(fqcn, false, classLoader)
22+
true
23+
} catch (_: ClassNotFoundException) {
24+
false
25+
}
26+
}
27+
withClue(
28+
"None of ${OutboxAutoConfiguration.JPA_HIBERNATE_PTM_CLASSES} resolves — the JPA branch " +
29+
"of extractDataSource is now dead code (Spring likely renamed a class or moved a " +
30+
"package). Update the set.",
31+
) {
32+
resolvable.shouldNotBeEmpty()
33+
}
34+
}
35+
})

‎okapi-spring-boot/src/main/kotlin/com/softwaremill/okapi/springboot/OutboxAutoConfiguration.kt‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,10 @@ class OutboxAutoConfiguration(
109109
): TransactionRunner {
110110
val anyTemplate = transactionTemplate.getIfUnique()
111111
// Extract the PTM from the TT if available, else resolve through the provider. TT's
112-
// transactionManager is nullable in the API (a TT can be constructed without one) — fall
113-
// back to the provider path in that pathological case so we still get a validated PTM.
112+
// transactionManager is nullable in the API, but Spring's InitializingBean contract rejects
113+
// a TransactionTemplate bean with a null transactionManager at afterPropertiesSet() (proven
114+
// by the PROBE test in OutboxAutoConfigurationTransactionRunnerTest). The `?:` is therefore
115+
// an unreachable-via-Spring defensive guard, kept correct for any non-Spring caller.
114116
val ptm = anyTemplate?.transactionManager
115117
?: resolvePlatformTransactionManager(transactionManager, beanFactory, okapiProperties)
116118
validatePtmDataSourceMatch(ptm, resolveDataSource(), okapiProperties)
@@ -321,21 +323,21 @@ class OutboxAutoConfiguration(
321323
val unwrappedOutbox = unwrapDataSource(outboxDataSource)
322324
if (unwrappedPtm !== unwrappedOutbox) {
323325
// If either side is still a DelegatingDataSource after unwrap, the chain terminated
324-
// early — either a cycle (`setTargetDataSource(self)`) or a not-yet-initialised
325-
// `LazyConnectionDataSourceProxy.targetDataSource == null`. Surface that as a distinct
326-
// WARN so the operator looks at the proxy wiring instead of chasing the PTM↔DS error.
326+
// early — a cycle (`setTargetDataSource(self)`) or a not-yet-initialised
327+
// `LazyConnectionDataSourceProxy.targetDataSource == null`. In that case the `!==`
328+
// is inconclusive: we never reached the concrete backing DataSources, so we do NOT
329+
// know whether they actually differ. Throwing the PTM↔DS mismatch error here would
330+
// assert something we did not verify and send the operator chasing the wrong fix
331+
// (qualifier config) instead of the real one (proxy wiring). Raise a distinct error.
327332
if (unwrappedPtm is DelegatingDataSource || unwrappedOutbox is DelegatingDataSource) {
328-
logger.warn(
329-
"Could not fully unwrap one or both DataSource sides — at least one " +
330-
"DelegatingDataSource chain terminated early (cycle, or " +
331-
"LazyConnectionDataSourceProxy with targetDataSource not yet set). " +
332-
"PTM side: {} (stopped at {}). Outbox side: {} (stopped at {}). If the " +
333-
"two are intended to wrap the same DataSource, fix the proxy chain " +
334-
"before relying on the PTM↔DataSource mismatch error below.",
335-
ptmDataSource,
336-
unwrappedPtm,
337-
outboxDataSource,
338-
unwrappedOutbox,
333+
error(
334+
"Could not verify the PTM↔DataSource binding: a DelegatingDataSource chain " +
335+
"terminated early — a cycle (setTargetDataSource(self)) or an " +
336+
"uninitialised LazyConnectionDataSourceProxy (targetDataSource not yet " +
337+
"set). PTM side: $ptmDataSource (stopped at $unwrappedPtm). Outbox side: " +
338+
"$outboxDataSource (stopped at $unwrappedOutbox). This is NOT necessarily " +
339+
"a DataSource mismatch — fix the proxy wiring so both chains resolve to a " +
340+
"concrete DataSource, or define an explicit @Bean TransactionRunner.",
339341
)
340342
}
341343
error(
@@ -397,25 +399,23 @@ class OutboxAutoConfiguration(
397399

398400
// JPA/Hibernate PTMs that expose a `public DataSource getDataSource()` — reflection by name
399401
// avoids requiring spring-orm on the compile classpath (it's optional for JDBC-only consumers).
400-
// Hibernate's TM moved package in Spring 6.2 — both names are listed so a single okapi build
401-
// works against Spring Framework 6.1.x and 6.2+ without a version matrix.
402-
// Hibernate's TM is named `org.springframework.orm.jpa.hibernate.HibernateTransactionManager`
403-
// in Spring 6.2+ and `org.springframework.orm.hibernate5.HibernateTransactionManager` in
404-
// Spring 6.1-. Both are listed so a single build works across versions.
405-
// `internal` so reflection-resolution tests can verify the set isn't all stale.
406-
internal val JPA_HIBERNATE_PTM_CLASSES = setOf(
402+
// Hibernate's TM is `org.springframework.orm.jpa.hibernate.HibernateTransactionManager` in
403+
// Spring 6.2+ and `org.springframework.orm.hibernate5.HibernateTransactionManager` in 6.1-;
404+
// both are listed so a single build works across versions without a matrix.
405+
// Public so the cross-module reflection-resolution guard test (in okapi-integration-tests,
406+
// where spring-orm is available) can verify the set isn't entirely stale.
407+
val JPA_HIBERNATE_PTM_CLASSES = setOf(
407408
"org.springframework.orm.jpa.JpaTransactionManager",
408409
"org.springframework.orm.jpa.hibernate.HibernateTransactionManager",
409410
"org.springframework.orm.hibernate5.HibernateTransactionManager",
410411
)
411412

412413
internal fun extractDataSource(ptm: PlatformTransactionManager): DataSource? {
413-
// Strategy 1: ResourceTransactionManager (DST + any RTM whose resourceFactory is a DataSource).
414414
(ptm as? ResourceTransactionManager)?.resourceFactory?.let { rf ->
415415
if (rf is DataSource) return rf
416416
}
417-
// Strategy 2/3: JPA/Hibernate public `getDataSource()`. Narrow catch on
418-
// NoSuchMethodException only — all other exceptions intentionally propagate:
417+
// JPA/Hibernate public `getDataSource()`. Narrow catch on NoSuchMethodException only —
418+
// all other exceptions intentionally propagate:
419419
// LinkageError / NoClassDefFoundError → mixed-jar classpath bug, must surface (this
420420
// is why we don't use `runCatching`, which would swallow them).
421421
// InvocationTargetException → JpaTransactionManager constructed without an EMF;

‎okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/DataSourceQualifierAutoConfigurationTest.kt‎

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,16 @@
11
package com.softwaremill.okapi.springboot
22

3-
import com.softwaremill.okapi.core.DeliveryResult
43
import com.softwaremill.okapi.core.MessageDeliverer
5-
import com.softwaremill.okapi.core.OutboxEntry
6-
import com.softwaremill.okapi.core.OutboxStatus
74
import com.softwaremill.okapi.core.OutboxStore
85
import com.softwaremill.okapi.core.TransactionRunner
96
import io.kotest.core.spec.style.FunSpec
107
import io.kotest.matchers.nulls.shouldNotBeNull
118
import io.kotest.matchers.string.shouldContain
129
import io.kotest.matchers.types.shouldBeSameInstanceAs
13-
import org.springframework.beans.factory.support.BeanDefinitionBuilder
1410
import org.springframework.boot.autoconfigure.AutoConfigurations
1511
import org.springframework.boot.test.context.runner.ApplicationContextRunner
1612
import org.springframework.context.support.GenericApplicationContext
1713
import org.springframework.jdbc.datasource.SimpleDriverDataSource
18-
import java.time.Instant
1914
import javax.sql.DataSource
2015

2116
class DataSourceQualifierAutoConfigurationTest : FunSpec({
@@ -41,10 +36,7 @@ class DataSourceQualifierAutoConfigurationTest : FunSpec({
4136
val outboxDs = SimpleDriverDataSource()
4237
baseRunner()
4338
.withInitializer { context ->
44-
val bd = BeanDefinitionBuilder.genericBeanDefinition(DataSource::class.java) { primaryDs }
45-
.beanDefinition
46-
bd.isPrimary = true
47-
(context as GenericApplicationContext).registerBeanDefinition("primaryDs", bd)
39+
(context as GenericApplicationContext).registerBean<DataSource>("primaryDs", primary = true) { primaryDs }
4840
}
4941
.withBean("outboxDs", DataSource::class.java, { outboxDs })
5042
.withPropertyValues("okapi.datasource-qualifier=outboxDs")
@@ -74,10 +66,7 @@ class DataSourceQualifierAutoConfigurationTest : FunSpec({
7466
val secondaryDs = SimpleDriverDataSource()
7567
baseRunner()
7668
.withInitializer { context ->
77-
val bd = BeanDefinitionBuilder.genericBeanDefinition(DataSource::class.java) { primaryDs }
78-
.beanDefinition
79-
bd.isPrimary = true
80-
(context as GenericApplicationContext).registerBeanDefinition("primaryDs", bd)
69+
(context as GenericApplicationContext).registerBean<DataSource>("primaryDs", primary = true) { primaryDs }
8170
}
8271
.withBean("secondaryDs", DataSource::class.java, { secondaryDs })
8372
.run { ctx ->
@@ -94,17 +83,3 @@ class DataSourceQualifierAutoConfigurationTest : FunSpec({
9483
private fun noOpTransactionRunner() = object : TransactionRunner {
9584
override fun <T> runInTransaction(block: () -> T): T = block()
9685
}
97-
98-
private fun stubStore() = object : OutboxStore {
99-
override fun persist(entry: OutboxEntry) = entry
100-
override fun claimPending(limit: Int) = emptyList<OutboxEntry>()
101-
override fun updateAfterProcessing(entry: OutboxEntry) = entry
102-
override fun removeDeliveredBefore(time: Instant, limit: Int) = 0
103-
override fun findOldestCreatedAt(statuses: Set<OutboxStatus>) = emptyMap<OutboxStatus, Instant>()
104-
override fun countByStatuses() = emptyMap<OutboxStatus, Long>()
105-
}
106-
107-
private fun stubDeliverer() = object : MessageDeliverer {
108-
override val type = "stub"
109-
override fun deliver(entry: OutboxEntry) = DeliveryResult.Success
110-
}

‎okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/LiquibaseAutoConfigurationTest.kt‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,7 @@ import ch.qos.logback.classic.Level
44
import ch.qos.logback.classic.Logger
55
import ch.qos.logback.classic.spi.ILoggingEvent
66
import ch.qos.logback.core.read.ListAppender
7-
import com.softwaremill.okapi.core.DeliveryResult
87
import com.softwaremill.okapi.core.MessageDeliverer
9-
import com.softwaremill.okapi.core.OutboxEntry
10-
import com.softwaremill.okapi.core.OutboxStatus
118
import com.softwaremill.okapi.core.OutboxStore
129
import com.softwaremill.okapi.core.TransactionRunner
1310
import io.kotest.assertions.throwables.shouldThrow
@@ -31,7 +28,6 @@ import org.springframework.boot.test.context.FilteredClassLoader
3128
import org.springframework.boot.test.context.runner.ApplicationContextRunner
3229
import org.springframework.core.annotation.AnnotatedElementUtils
3330
import org.springframework.jdbc.datasource.SimpleDriverDataSource
34-
import java.time.Instant
3531
import javax.sql.DataSource
3632
import io.kotest.matchers.string.shouldContain as stringShouldContain
3733

@@ -518,17 +514,3 @@ private fun canLoadClass(fqcn: String, classLoader: ClassLoader): Boolean = try
518514
private fun noOpTransactionRunner() = object : TransactionRunner {
519515
override fun <T> runInTransaction(block: () -> T): T = block()
520516
}
521-
522-
private fun stubStore() = object : OutboxStore {
523-
override fun persist(entry: OutboxEntry) = entry
524-
override fun claimPending(limit: Int) = emptyList<OutboxEntry>()
525-
override fun updateAfterProcessing(entry: OutboxEntry) = entry
526-
override fun removeDeliveredBefore(time: Instant, limit: Int) = 0
527-
override fun findOldestCreatedAt(statuses: Set<OutboxStatus>) = emptyMap<OutboxStatus, Instant>()
528-
override fun countByStatuses() = emptyMap<OutboxStatus, Long>()
529-
}
530-
531-
private fun stubDeliverer() = object : MessageDeliverer {
532-
override val type = "stub"
533-
override fun deliver(entry: OutboxEntry) = DeliveryResult.Success
534-
}

‎okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/LiquibaseE2ETest.kt‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
package com.softwaremill.okapi.springboot
22

33
import com.mysql.cj.jdbc.MysqlDataSource
4-
import com.softwaremill.okapi.core.DeliveryResult
54
import com.softwaremill.okapi.core.MessageDeliverer
6-
import com.softwaremill.okapi.core.OutboxEntry
75
import com.softwaremill.okapi.mysql.MysqlOutboxStore
86
import com.softwaremill.okapi.postgres.PostgresOutboxStore
97
import io.kotest.core.spec.style.FunSpec
@@ -413,8 +411,3 @@ private fun resolveSpringBootClass(vararg candidateFqcns: String): Class<*>? {
413411
}
414412
}
415413
}
416-
417-
private fun stubDeliverer() = object : MessageDeliverer {
418-
override val type = "stub"
419-
override fun deliver(entry: OutboxEntry) = DeliveryResult.Success
420-
}

‎okapi-spring-boot/src/test/kotlin/com/softwaremill/okapi/springboot/OkapiSpringTestStubs.kt‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package com.softwaremill.okapi.springboot
2+
3+
import com.softwaremill.okapi.core.DeliveryResult
4+
import com.softwaremill.okapi.core.MessageDeliverer
5+
import com.softwaremill.okapi.core.OutboxEntry
6+
import com.softwaremill.okapi.core.OutboxStatus
7+
import com.softwaremill.okapi.core.OutboxStore
8+
import org.springframework.beans.factory.support.BeanDefinitionBuilder
9+
import org.springframework.context.support.GenericApplicationContext
10+
import java.time.Instant
11+
12+
/**
13+
* Shared no-op test doubles + a bean-registration helper for okapi-spring-boot autoconfig slice
14+
* tests. Previously these were copy-pasted verbatim into 7+ test files; a single source means an
15+
* `OutboxStore` interface change is a one-line edit, not a 7-file sweep.
16+
*/
17+
18+
internal fun stubStore() = object : OutboxStore {
19+
override fun persist(entry: OutboxEntry) = entry
20+
override fun claimPending(limit: Int) = emptyList<OutboxEntry>()
21+
override fun updateAfterProcessing(entry: OutboxEntry) = entry
22+
override fun removeDeliveredBefore(time: Instant, limit: Int) = 0
23+
override fun findOldestCreatedAt(statuses: Set<OutboxStatus>) = emptyMap<OutboxStatus, Instant>()
24+
override fun countByStatuses() = emptyMap<OutboxStatus, Long>()
25+
}
26+
27+
internal fun stubDeliverer() = stubDelivererWithType("stub")
28+
29+
internal fun stubDelivererWithType(t: String) = object : MessageDeliverer {
30+
override val type = t
31+
override fun deliver(entry: OutboxEntry) = DeliveryResult.Success
32+
}
33+
34+
/**
35+
* Registers a bean via [GenericApplicationContext.registerBeanDefinition], optionally `@Primary`.
36+
* Collapses the `BeanDefinitionBuilder.genericBeanDefinition(...).beanDefinition.apply { isPrimary }`
37+
* boilerplate that the multi-DataSource tests repeat for every DataSource / PTM bean.
38+
*/
39+
internal inline fun <reified T : Any> GenericApplicationContext.registerBean(
40+
name: String,
41+
primary: Boolean = false,
42+
crossinline supplier: () -> T,
43+
) {
44+
registerBeanDefinition(
45+
name,
46+
BeanDefinitionBuilder.genericBeanDefinition(T::class.java) { supplier() }.beanDefinition
47+
.apply { isPrimary = primary },
48+
)
49+
}

0 commit comments

Comments
 (0)