Aman-Mittal commented on code in PR #6025: URL: https://github.com/apache/fineract/pull/6025#discussion_r3480332625
########## fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/config/TransactionManagerDataSourceInvariantTest.java: ########## @@ -0,0 +1,99 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.core.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import java.util.List; +import org.apache.fineract.infrastructure.core.config.jpa.JpaTransactionConfig; +import org.apache.fineract.infrastructure.core.persistence.TransactionLifecycleCallback; +import org.apache.fineract.infrastructure.core.service.database.RoutingDataSource; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.boot.autoconfigure.transaction.TransactionManagerCustomizers; +import org.springframework.jdbc.datasource.DataSourceTransactionManager; +import org.springframework.orm.jpa.JpaTransactionManager; +import org.springframework.transaction.PlatformTransactionManager; + +/** + * Locks in the invariant that keeps mixed JPA/JDBC usage consistent: the primary (JPA) transaction manager must NOT + * have a {@link javax.sql.DataSource} bound, while the dedicated JDBC transaction manager must. + * + * <p> + * Why this matters: Spring's {@link JpaTransactionManager} only binds the EclipseLink JDBC connection as a thread-bound + * resource (so that a {@code JdbcTemplate} on the same {@code DataSource} reuses it) when + * {@code getDataSource() != null}. Fineract deliberately leaves the JPA manager's {@code DataSource} unset, so plain + * JDBC access pulls its own connection from the pool and does not share EclipseLink's transactional connection. That + * separation is precisely what makes it safe to run EclipseLink with {@code lazyDatabaseTransaction = true} (which + * defers - and with the lock-free dialect, unlocks - acquisition of EclipseLink's own JDBC connection): there is no + * JPA/JDBC connection sharing to compromise. + * + * <p> + * If someone later calls {@code setDataSource(...)} on the JPA transaction manager to make JPA and JDBC atomic, this + * test fails on purpose - that change would reintroduce connection sharing and {@code lazyDatabaseTransaction} would + * then be the wrong setting (eager mode would be required instead). + */ +public class TransactionManagerDataSourceInvariantTest { + + @Test + void jpaTransactionManagerMustNotHaveDataSourceSoJdbcDoesNotShareEclipseLinkConnection() { + PlatformTransactionManager transactionManager = new JpaTransactionConfig().jpaTransactionManager(writeModeProperties(), + noTransactionManagerCustomizers(), noCallbacks()); + + assertThat(transactionManager).isInstanceOf(JpaTransactionManager.class); + assertThat(((JpaTransactionManager) transactionManager).getDataSource()) + .as("JPA transaction manager must not bind a DataSource; otherwise JdbcTemplate would share EclipseLink's " + + "JDBC connection and lazyDatabaseTransaction could compromise JPA/JDBC consistency") + .isNull(); + } + + @Test + public void jdbcTransactionManagerMustBindDataSourceSoPlainJdbcParticipatesInTransactions() { + RoutingDataSource dataSource = mock(RoutingDataSource.class); + + PlatformTransactionManager transactionManager = new JdbcTransactionConfig().jdbcTransactionManager(writeModeProperties(), + dataSource, noTransactionManagerCustomizers(), noCallbacks()); + + assertThat(transactionManager).isInstanceOf(DataSourceTransactionManager.class); + assertThat(((DataSourceTransactionManager) transactionManager).getDataSource()) + .as("JDBC transaction manager must bind the routing DataSource so JdbcTemplate work participates in its transaction") + .isSameAs(dataSource); + } + + private static FineractProperties writeModeProperties() { + FineractProperties properties = new FineractProperties(); + FineractProperties.FineractModeProperties mode = new FineractProperties.FineractModeProperties(); + mode.setWriteEnabled(true); + properties.setMode(mode); + return properties; + } + + // Mirrors production: Spring Boot's default TransactionManagerCustomizers do not set a DataSource on the manager. + // ifAvailable(...) on a Mockito mock is a no-op, so no customizer is applied - we assert the config's own + // behaviour. + @SuppressWarnings("unchecked") Review Comment: Why are we suppressing warnings ########## fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/persistence/visibility/JpaJdbcSameTransactionVisibilityTest.java: ########## @@ -0,0 +1,252 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.infrastructure.core.persistence.visibility; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.zaxxer.hikari.HikariDataSource; +import jakarta.persistence.EntityManager; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import javax.sql.DataSource; +import org.apache.fineract.infrastructure.core.persistence.ExtendedJpaTransactionManager; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.orm.jpa.EntityManagerFactoryUtils; +import org.springframework.orm.jpa.EntityManagerHolder; +import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean; +import org.springframework.orm.jpa.vendor.EclipseLinkJpaVendorAdapter; +import org.springframework.transaction.support.TransactionSynchronizationManager; +import org.springframework.transaction.support.TransactionTemplate; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; + +/** + * Real (non-mocked) test of whether JPA (EclipseLink {@link EntityManager}) and direct JDBC ({@link JdbcTemplate}) run + * on the same physical connection inside one transaction - and therefore whether they can read each other's + * flushed-but-not-yet-committed writes. It runs against a real PostgreSQL (via Testcontainers) with a real EclipseLink + * EntityManagerFactory, a real {@link ExtendedJpaTransactionManager}, and a real {@link JdbcTemplate} on the same + * {@link DataSource}. + * + * <p> + * Confirmed finding: within one transaction, JPA and JDBC share a single physical connection, so JdbcTemplate reads see + * the transaction's flushed-but-uncommitted JPA writes and both commit/roll back atomically together. This holds both + * with and without an explicit {@code setDataSource(...)} on the transaction manager - i.e. binding the DataSource is + * NOT what causes the sharing; {@link org.springframework.orm.jpa.JpaTransactionManager} makes EclipseLink's connection + * available to plain JDBC regardless (it derives the DataSource from the EntityManagerFactory). Because the connection + * is shared once materialised, running EclipseLink with {@code lazyDatabaseTransaction=true} is safe: lazy mode only + * defers <em>when</em> that single connection is acquired, not whether JPA and JDBC share it. + * + * <p> + * The sharing is established two ways for certainty: + * <ul> + * <li>comparing {@code pg_backend_pid()} (unique per PostgreSQL backend/connection) obtained via the EntityManager vs + * via the JdbcTemplate - equal PIDs prove the same physical connection, and</li> + * <li>a control query on a genuinely separate raw connection taken directly from the pool (bypassing Spring): under + * READ_COMMITTED it must NOT see the uncommitted row, proving the JPA write is truly uncommitted and ruling out a + * premature/auto-commit explanation for the visibility observed via JdbcTemplate.</li> + * </ul> + * + * <p> + * Requires a Docker daemon; the whole class is skipped automatically when Docker is unavailable. + */ +@Testcontainers(disabledWithoutDocker = true) +public class JpaJdbcSameTransactionVisibilityTest { + + @Container + private static final PostgreSQLContainer<?> POSTGRES = new PostgreSQLContainer<>("postgres:16-alpine"); + + private static final AtomicInteger DB_COUNTER = new AtomicInteger(); + + private final List<LocalContainerEntityManagerFactoryBean> createdFactories = new ArrayList<>(); + private final List<HikariDataSource> createdDataSources = new ArrayList<>(); + + @AfterEach + public void tearDown() { + createdFactories.forEach(LocalContainerEntityManagerFactoryBean::destroy); + createdFactories.clear(); + createdDataSources.forEach(HikariDataSource::close); + createdDataSources.clear(); + } + + @Test + public void withoutDataSourceBoundOnManager() { + ScenarioResult result = runScenario(createFixture(false)); + + assertResultsAreConsistent(result); + // Mirrors Fineract's primary transaction manager (no explicit setDataSource). JPA and JDBC still run on the + // same + // physical PostgreSQL backend: JpaTransactionManager exposes EclipseLink's connection to plain JDBC even + // without + // an explicit DataSource binding (it derives the DataSource from the EntityManagerFactory). + assertThat(result.jpaBackendPid()) + .as("JdbcTemplate must run on the same physical PostgreSQL backend as EclipseLink, so it can read the " + + "JPA transaction's uncommitted writes") + .isEqualTo(result.jdbcBackendPid()); + assertThat(result.jdbcCountInsideTransaction()) + .as("sharing the connection, JdbcTemplate sees the flushed-but-uncommitted JPA row in the same transaction").isEqualTo(1); + } + + @Test + public void withDataSourceBoundOnManager() { + ScenarioResult result = runScenario(createFixture(true)); + + assertResultsAreConsistent(result); + assertThat(result.jpaBackendPid()).as("with the DataSource bound, JdbcTemplate shares EclipseLink's physical connection") + .isEqualTo(result.jdbcBackendPid()); + assertThat(result.jdbcCountInsideTransaction()) + .as("sharing the connection, JdbcTemplate sees the flushed-but-uncommitted JPA row in the same transaction").isEqualTo(1); + } + + /** + * Invariants that hold regardless of connection sharing: the JPA write is real but genuinely uncommitted (a + * separate raw connection cannot see it), and it becomes visible to everyone only after commit. The + * separate-connection check is the decisive control that rules out a premature/auto-commit explanation for any + * visibility observed elsewhere. + */ + private void assertResultsAreConsistent(ScenarioResult result) { + assertThat(result.jpaCountInsideTransaction()).as("JPA must see its own flushed row").isEqualTo(1L); + assertThat(result.separateRawConnectionCountInsideTransaction()) + .as("a genuinely separate connection (taken straight from the pool, outside Spring) must NOT see the row - " Review Comment: minor: we can consider using java textblock here -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
