diff --git a/pom.xml b/pom.xml index c6ba065ce..5b6a0731b 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ org.dbunit dbunit - 3.5.1 + 3.5.2-SNAPSHOT jar dbUnit Extension https://github.com/dbunit/dbunit-extension diff --git a/src/changes/changes.xml b/src/changes/changes.xml index df373ad62..e6e5daa0e 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -13,6 +13,11 @@ + + + Fix DefaultPrepAndExpectedTestCase reusing a closed connection when closeConnectionAfterTest is false and one instance is reused across test methods (a base-class or shared static field paired with a CachingConnectionProvider): getReusableConnection() pinned the first IDatabaseConnection in a field and closeReusableConnection() kept it there without closing, so a connection the pool or database dropped between tests - max-lifetime/reap, a PGConnectionPoolDataSource issuing a fresh handle, a bounced application context, an idle-in-transaction kill - left every following test failing on it, in setupData()'s CLEAN_INSERT and then cleanupData()'s tear down operation, with CachingConnectionProvider's own liveness check never reached to replace it. getReusableConnection() now discards a closed cached connection before returning it, and closeReusableConnectionSuppressing() forgets the connection after any failed lifecycle step, so the next test re-acquires a live one from the tester. Present since 3.4.0, when setCloseConnectionAfterTest() was added. + + Fix a 3.5.0 regression (introduced by issue #496's fix) where FlatXmlProducer added every table from an explicitly-supplied metadata IDataSet as an empty table, not just tables actually present in the flat XML body: FlatXmlDataSetBuilder#setMetaDataSet(IDataSet) is documented as supplying column metadata only, but a broad metadata source such as a live database's full IDataSet (e.g. via DatabaseConnection#createDataSet()) was being read as if it enumerated the fixture's own tables, so DELETE_ALL/CLEAN_INSERT ended up touching every table in that broader source instead of only the ones the XML body mentions. FlatXmlProducer's issue #496 empty-table backfill now only runs when the metadata source is DTD-derived (a FlatDtdDataSet, whether parsed inline from the flat XML's own DOCTYPE or supplied via FlatXmlDataSetBuilder#setMetaDataSetFromDtd), restoring pre-3.5.0 behavior for any other explicitly-supplied metadata dataset. diff --git a/src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java b/src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java index d3b0400c9..409afce0d 100644 --- a/src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java +++ b/src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java @@ -20,6 +20,7 @@ */ package org.dbunit; +import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; @@ -278,6 +279,20 @@ private boolean lookupFeatureValue(final String featureName) * Return the connection shared by lookupFeatureValue(), setupData(), * verifyData() and cleanupData() for the current test's lifecycle, * acquiring it on first use instead of a fresh connection at each step. + *

+ * When {@link #closeConnectionAfterTest} is false this connection is kept + * across test methods (see {@link #closeReusableConnection()}), where the + * connection pool or the database server can close it between tests - a + * pool max-lifetime or reap, a bounced application context, a + * {@link org.dbunit.database.CachingConnectionProvider#close()}. A closed + * one is discarded here before it is handed back, so the next call + * re-acquires from databaseTester - letting a + * {@link org.dbunit.database.CachingConnectionProvider} behind it supply a + * live replacement - rather than this instance reusing a connection every + * later lifecycle step would only fail on. With + * {@link #closeConnectionAfterTest} left at its default the connection is + * closed and forgotten after each test anyway, so it is not re-checked + * here. * * @return The shared connection. * @throws Exception On dbUnit errors. @@ -285,6 +300,11 @@ private boolean lookupFeatureValue(final String featureName) */ private IDatabaseConnection getReusableConnection() throws Exception { + if (connection != null && !closeConnectionAfterTest + && isReusableConnectionClosed()) + { + connection = null; + } if (connection == null) { connection = getConnection(); @@ -292,13 +312,46 @@ private IDatabaseConnection getReusableConnection() throws Exception return connection; } + /** + * Returns whether the connection currently cached in {@link #connection} has + * been closed - by the connection pool, the database server, or a + * {@link org.dbunit.database.CachingConnectionProvider} behind + * {@code databaseTester} - since this instance last used it. A connection + * that throws while being asked is treated as closed. Uses only the local + * {@link Connection#isClosed()} flag, not a round-tripping + * {@link Connection#isValid(int)}: cheap, side-effect free, and enough to + * catch a connection closed between test methods before the next one's + * first statement. A server-side disconnect the driver has not noticed yet + * instead surfaces once, when a lifecycle step runs a statement against it; + * {@link #closeReusableConnectionSuppressing(Throwable)} then forgets the + * connection so the following test re-acquires regardless. + * + * @return True when the cached connection is known to be closed or unusable. + * @since 3.5.2 + */ + private boolean isReusableConnectionClosed() + { + try + { + return connection.getConnection().isClosed(); + } catch (final SQLException e) + { + log.debug("isReusableConnectionClosed: treating the cached connection" + + " as closed after it failed to report its state", e); + return true; + } + } + /** * Release the connection shared by lookupFeatureValue(), setupData(), * verifyData() and cleanupData(), if one was acquired: closes it and * forgets it when {@link #closeConnectionAfterTest} is true (the * default); otherwise leaves it open and keeps the field set, so a later * lifecycle step's getReusableConnection() call keeps reusing it rather - * than acquiring - and silently orphaning - another one. + * than acquiring - and silently orphaning - another one. That later call + * still drops the kept connection if it has since died (see + * {@link #getReusableConnection()}), so a connection the pool or server + * closed between test methods does not linger to fail every following one. * * @throws SQLException On close errors. * @since 3.4.0 @@ -333,6 +386,14 @@ private void closeReusableConnection() throws SQLException * {@link Throwable#addSuppressed(Throwable)} rather than letting it * replace and hide the primary. Mirrors the exception safety of * {@link #runTest} and {@code DatabaseTestCase.tearDown(Throwable)}. + *

+ * Only ever called from a lifecycle step that has already failed, so it + * also forgets the shared connection even when {@link #closeConnectionAfterTest} + * is false and {@link #closeReusableConnection()} therefore left it open: a + * step that just threw may have broken it, so the next + * {@link #getReusableConnection()} re-acquires rather than reusing it. Any + * {@link org.dbunit.database.CachingConnectionProvider} behind + * {@code databaseTester} still owns closing it. * * @param primary * The exception already in flight to attach a close failure @@ -348,6 +409,7 @@ private void closeReusableConnectionSuppressing(final Throwable primary) { primary.addSuppressed(closeFailure); } + connection = null; } /** diff --git a/src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java b/src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java index 228966fa8..6c8dc2c6e 100644 --- a/src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java +++ b/src/test/java/org/dbunit/DatabaseTesterConnectionReuseIT.java @@ -205,12 +205,100 @@ void testDefaultPrepAndExpectedTestCase_acrossFreshInstancesSharingAProviderWith } } + @Test + void testDefaultPrepAndExpectedTestCase_reusedAcrossTestMethodsWithCloseDisabled_keepsReusingItsOneOpenConnection() + throws Exception + { + final CachingConnectionProvider provider = new CachingConnectionProvider(); + final IDatabaseTester tester = newSharedProviderTester(provider); + tester.setTearDownOperation(DatabaseOperation.DELETE_ALL); + final DefaultPrepAndExpectedTestCase tc = newCloseDisabledTestCase(tester); + final List connectionsSeen = new ArrayList<>(); + try + { + for (int simulatedTestMethod = 0; simulatedTestMethod < 3; simulatedTestMethod++) + { + runOneSimulatedTestMethod(tc); + + final IDatabaseConnection connection = tester.getConnection(); + connectionsSeen.add(connection); + assertThat(connection.getConnection().isClosed()) + .as("closeConnectionAfterTest=false: the connection must still be" + + " open for the next simulated test method to reuse.") + .isFalse(); + } + + assertThat(new HashSet<>(connectionsSeen)) + .as("One DefaultPrepAndExpectedTestCase reused across test methods, backed" + + " by a CachingConnectionProvider, must run every method against" + + " the one cached connection while it stays alive.") + .hasSize(1); + } finally + { + provider.close(); + } + } + + @Test + void testDefaultPrepAndExpectedTestCase_reusedWithCloseDisabled_whenItsCachedConnectionDiesBetweenTestMethods_replacesItRatherThanReusingTheDeadOne() + throws Exception + { + final CachingConnectionProvider provider = new CachingConnectionProvider(); + final IDatabaseTester tester = newSharedProviderTester(provider); + tester.setTearDownOperation(DatabaseOperation.DELETE_ALL); + final DefaultPrepAndExpectedTestCase tc = newCloseDisabledTestCase(tester); + try + { + // First simulated test method: acquires and pins a connection. + runOneSimulatedTestMethod(tc); + final IDatabaseConnection firstConnection = tester.getConnection(); + + // The connection pool or the database server drops that connection + // between test methods - max lifetime, an idle-in-transaction + // timeout, a bounced application context, and so on. + firstConnection.getConnection().close(); + + // Second simulated test method on the SAME instance must notice the + // dead connection it pinned and acquire a live replacement, not fail + // setupData()'s CLEAN_INSERT - and then cleanupData()'s tear down + // operation - on the connection it can no longer use. + runOneSimulatedTestMethod(tc); + + final IDatabaseConnection secondConnection = tester.getConnection(); + assertThat(secondConnection.getConnection().isClosed()) + .as("The reused test case must have replaced the connection killed" + + " between test methods, not kept handing back the dead one.") + .isFalse(); + assertThat(secondConnection) + .as("A fresh connection must have been acquired from the" + + " CachingConnectionProvider once the first one died.") + .isNotSameAs(firstConnection); + } finally + { + provider.close(); + } + } + private DefaultPrepAndExpectedTestCase newTestCase(final IDatabaseTester tester) { final DataFileLoader dataFileLoader = new FlatXmlDataFileLoader(); return new DefaultPrepAndExpectedTestCase(dataFileLoader, tester); } + private DefaultPrepAndExpectedTestCase newCloseDisabledTestCase(final IDatabaseTester tester) + { + final DefaultPrepAndExpectedTestCase tc = newTestCase(tester); + tc.setCloseConnectionAfterTest(false); + return tc; + } + + private static void runOneSimulatedTestMethod(final DefaultPrepAndExpectedTestCase tc) + throws Exception + { + tc.runTest(new VerifyTableDefinition[] {}, new String[] {}, new String[] {}, + () -> null); + } + private IDatabaseTester newSharedProviderTester(final CachingConnectionProvider sharedProvider) throws Exception {