diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java index fc705c10..156a98b4 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java @@ -5,8 +5,6 @@ import java.util.HashSet; import java.util.Set; import org.hibernate.Hibernate; -import org.hibernate.SessionFactory; -import org.hibernate.stat.Statistics; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager; @@ -15,7 +13,7 @@ import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest; import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; -import jakarta.persistence.EntityManager; +import com.digitalsanctuary.spring.user.test.config.StatementCountInspector; /** * Database-slice tests verifying that {@link UserRepository#findWithRolesByEmail(String)} eagerly loads the @@ -62,13 +60,6 @@ private void persistUserWithRolesAndPrivileges(String email) { entityManager.clear(); } - private Statistics statistics() { - EntityManager em = entityManager.getEntityManager(); - Statistics stats = em.getEntityManagerFactory().unwrap(SessionFactory.class).getStatistics(); - stats.setStatisticsEnabled(true); - return stats; - } - @Test void shouldInitializeRolesAndPrivilegesWhenLoadedViaEntityGraphFinder() { persistUserWithRolesAndPrivileges("graph@test.com"); @@ -121,8 +112,9 @@ void shouldNotInitializeRolesWhenLoadedViaPlainFindByEmail() { void shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder() { persistUserWithRolesAndPrivileges("bounded@test.com"); - Statistics stats = statistics(); - stats.clear(); + // Count prepared statements per-thread rather than reading the SessionFactory-global Statistics counter, + // which is polluted by tests running concurrently on other threads under JUnit parallel execution (GH-337). + StatementCountInspector.reset(); User user = userRepository.findWithRolesByEmail("bounded@test.com"); // Force full traversal to prove no additional (N+1) queries are issued for privileges. @@ -132,10 +124,12 @@ void shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder() { } assertThat(privilegeCount).isPositive(); - // A single entity-graph fetch should not degenerate into a per-role / per-privilege N+1 explosion. - // Allow a small bound to tolerate join-table fetch strategy differences across Hibernate versions. - assertThat(stats.getPrepareStatementCount()) + // Upper bound: a single entity-graph fetch should not degenerate into a per-role / per-privilege N+1 + // explosion. A small bound tolerates join-table fetch strategy differences across Hibernate versions. + // Lower bound: at least one statement must be counted, so a broken/unregistered inspector (which would + // read 0) fails loudly instead of passing as a false green. + assertThat(StatementCountInspector.getCount()) .as("EntityGraph finder should load roles + privileges in a bounded number of queries") - .isLessThanOrEqualTo(3); + .isBetween(1, 3); } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspector.java b/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspector.java new file mode 100644 index 00000000..78599e5a --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspector.java @@ -0,0 +1,54 @@ +package com.digitalsanctuary.spring.user.test.config; + +import java.io.Serializable; +import org.hibernate.resource.jdbc.spi.StatementInspector; + +/** + * A Hibernate {@link StatementInspector} that counts prepared JDBC statements on a per-thread basis. + * + *

+ * The test suite runs with JUnit 5 parallel execution, and {@code @DataJpaTest} classes share a single cached Spring context (and therefore a + * single {@code SessionFactory} and its {@code SessionFactory}-global {@code Statistics} object). Reading + * {@code Statistics#getPrepareStatementCount()} to guard against N+1 query explosions is therefore polluted by statements issued from tests running + * concurrently on other threads: the counter is neither session-scoped nor transactional, so {@code @DataJpaTest} rollback does not isolate it. + * + *

+ * Because each parallel test executes on its own thread and JDBC connection, this inspector records its count in a {@link ThreadLocal}, yielding a + * stable, per-test measurement that is immune to concurrent pollution. A single instance is registered on the shared test {@code SessionFactory} via + * the {@code hibernate.session_factory.statement_inspector} property (see {@code application-test.properties}); the counter state is {@code static} so + * the inspecting instance created by Hibernate and the assertions in a test share the same per-thread counter. + * + *

+ * Usage: call {@link #reset()} immediately before the code under measurement, then assert on {@link #getCount()}. + */ +public final class StatementCountInspector implements StatementInspector, Serializable { + + private static final long serialVersionUID = 1L; + + // The counter is intentionally left set (rather than removed) for the calling thread's lifetime: JUnit's parallel + // executor reuses a bounded worker-thread pool, so at most one boxed Integer lingers per pool thread, and every + // measurement calls reset() first, so a stale value from a prior test can never leak into a new one. + private static final ThreadLocal COUNT = ThreadLocal.withInitial(() -> 0); + + /** + * Resets the calling thread's statement counter to zero. + */ + public static void reset() { + COUNT.set(0); + } + + /** + * Returns the number of prepared statements inspected on the calling thread since the last {@link #reset()}. + * + * @return the per-thread prepared-statement count + */ + public static int getCount() { + return COUNT.get(); + } + + @Override + public String inspect(String sql) { + COUNT.set(COUNT.get() + 1); + return sql; + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspectorTest.java b/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspectorTest.java new file mode 100644 index 00000000..2c55f003 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspectorTest.java @@ -0,0 +1,99 @@ +package com.digitalsanctuary.spring.user.test.config; + +import static org.assertj.core.api.Assertions.assertThat; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link StatementCountInspector}. These exercise the counter mechanism directly (no Spring context, no Hibernate) so the thread-local + * reset/count semantics that isolate {@code UserRepositoryEntityGraphTest} from parallel-execution pollution are locked in independently of that one + * consumer test (GH-337). + */ +class StatementCountInspectorTest { + + @Test + void shouldReportZeroCountAfterReset() { + StatementCountInspector inspector = new StatementCountInspector(); + inspector.inspect("select 1"); + + StatementCountInspector.reset(); + + assertThat(StatementCountInspector.getCount()).isZero(); + } + + @Test + void shouldIncrementCountForEachInspectCall() { + StatementCountInspector inspector = new StatementCountInspector(); + StatementCountInspector.reset(); + + inspector.inspect("select 1"); + inspector.inspect("select 2"); + inspector.inspect("select 3"); + + assertThat(StatementCountInspector.getCount()).isEqualTo(3); + } + + @Test + void shouldReturnSqlUnchangedWhenInspecting() { + StatementCountInspector inspector = new StatementCountInspector(); + String sql = "select * from users where email = ?"; + + assertThat(inspector.inspect(sql)).isEqualTo(sql); + } + + /** + * The core property behind the GH-337 fix: one thread's counting — including its {@link StatementCountInspector#reset()} — must not perturb + * another thread's count. Two threads are interleaved so that thread B fully resets and counts after thread A has counted but + * before thread A reads its final value; thread A must still observe only its own statements. + */ + @Test + void shouldIsolateCountsBetweenThreads() throws InterruptedException { + StatementCountInspector inspector = new StatementCountInspector(); + CountDownLatch threadAHasCounted = new CountDownLatch(1); + CountDownLatch threadBIsDone = new CountDownLatch(1); + AtomicInteger threadAFinalCount = new AtomicInteger(-1); + AtomicInteger threadBFinalCount = new AtomicInteger(-1); + AtomicReference failure = new AtomicReference<>(); + + Thread threadA = new Thread(() -> { + try { + StatementCountInspector.reset(); + inspector.inspect("a1"); + inspector.inspect("a2"); + inspector.inspect("a3"); + threadAHasCounted.countDown(); + assertThat(threadBIsDone.await(5, TimeUnit.SECONDS)).isTrue(); + // Thread B has since reset and counted on its own thread; A's count must be untouched. + threadAFinalCount.set(StatementCountInspector.getCount()); + } catch (Throwable t) { + failure.compareAndSet(null, t); + } + }); + + Thread threadB = new Thread(() -> { + try { + assertThat(threadAHasCounted.await(5, TimeUnit.SECONDS)).isTrue(); + StatementCountInspector.reset(); + inspector.inspect("b1"); + inspector.inspect("b2"); + threadBFinalCount.set(StatementCountInspector.getCount()); + threadBIsDone.countDown(); + } catch (Throwable t) { + failure.compareAndSet(null, t); + threadBIsDone.countDown(); + } + }); + + threadA.start(); + threadB.start(); + threadA.join(TimeUnit.SECONDS.toMillis(10)); + threadB.join(TimeUnit.SECONDS.toMillis(10)); + + assertThat(failure.get()).isNull(); + assertThat(threadAFinalCount.get()).as("thread A must see only its own 3 statements").isEqualTo(3); + assertThat(threadBFinalCount.get()).as("thread B must see only its own 2 statements").isEqualTo(2); + } +} diff --git a/src/test/resources/application-test.properties b/src/test/resources/application-test.properties index b6a79f8e..e2f8ef01 100644 --- a/src/test/resources/application-test.properties +++ b/src/test/resources/application-test.properties @@ -17,6 +17,12 @@ spring.jpa.properties.hibernate.dialect=org.hibernate.dialect.H2Dialect spring.jpa.hibernate.ddl-auto=create-drop spring.jpa.show-sql=false spring.jpa.properties.hibernate.format_sql=true +# Per-thread prepared-statement counter used by UserRepositoryEntityGraphTest to guard against N+1 +# regressions without reading the SessionFactory-global Statistics counter (see GH-337). +# Registered profile-wide by design: it is the simplest place to wire a StatementInspector onto the +# shared test SessionFactory. Every JPA test therefore runs a single ThreadLocal increment per prepared +# statement, which is an inert, negligible overhead for tests that never read the counter. +spring.jpa.properties.hibernate.session_factory.statement_inspector=com.digitalsanctuary.spring.user.test.config.StatementCountInspector # Logging logging.level.com.digitalsanctuary.spring.user=INFO