This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e6a44d4a016e1460e7e00dfb250971bdca04f8b8 Author: opencode <[email protected]> AuthorDate: Tue Nov 3 09:49:13 2026 +0100 Treat a blank validation query as the unset value A validationQuery written blank is how a configuration surface that cannot express the unset value - a JNDI attribute, a connection-properties line - says "no statement", and an emptiness is not executable as SQL anyway: against a driver that refuses empty statements the pool retired and recreated every connection it touched, and against a permissive one it paid a pointless round trip per validation, from a configuration that looked benign. The pool's own convention already collapses a blank for the init SQL; the validation query now follows it. The setter stores a blank as the unset value, and the validation path reads a blank as unset even where the value reached it around that setter - a PoolConfiguration implementation of the caller's own, or an explicitly empty statement argument - running the built-in Connection#isValid check the unset value documents. --- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 10 +- .../apache/tomcat/jdbc/pool/PoolProperties.java | 15 +- .../apache/tomcat/jdbc/pool/PooledConnection.java | 29 ++- .../tomcat/jdbc/pool/TestBlankValidationQuery.java | 237 +++++++++++++++++++++ 4 files changed, 285 insertions(+), 6 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 715794ebd3..9393293a62 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -608,7 +608,9 @@ public interface PoolConfiguration { /** * The SQL query that will be used to validate connections from this pool before returning them to the caller or * pool. If specified, this query does not have to return any data, it just can't throw an SQLException. The default - * value is null. Example values are SELECT 1(mysql), select 1 from dual(oracle), SELECT 1(MS Sql Server) + * value is null. A blank query means the unset value: no statement, the built-in + * {@link java.sql.Connection#isValid(int)} check runs instead. Example values are SELECT 1(mysql), select 1 from + * dual(oracle), SELECT 1(MS Sql Server) * * @return the query used for validation or null if no validation is performed */ @@ -617,9 +619,11 @@ public interface PoolConfiguration { /** * The SQL query that will be used to validate connections from this pool before returning them to the caller or * pool. If specified, this query does not have to return any data, it just can't throw an SQLException. The default - * value is null. Example values are SELECT 1(mysql), select 1 from dual(oracle), SELECT 1(MS Sql Server) + * value is null. A blank query is stored as the unset value: a configuration surface that cannot express null is + * written empty to mean "no statement", and no statement can be blank. Example values are SELECT 1(mysql), select + * 1 from dual(oracle), SELECT 1(MS Sql Server) * - * @param validationQuery the query used for validation or null if no validation is performed + * @param validationQuery the query used for validation, or null or a blank string if no validation query is set */ void setValidationQuery(String validationQuery); diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index 02213982c8..ba0920408c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -835,9 +835,22 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Stores the validation query, treating a blank string as the unset value + * it means. A configuration surface that cannot express {@code null} - a + * JNDI attribute, a connection-properties line - is routinely written with + * an empty value to say "no statement, validate the way an unset query + * does", and a statement cannot be blank anyway; keeping such a value + * would make the pool execute the emptiness as SQL. This mirrors + * {@link #setInitSQL(String)}; a query that carries anything is stored + * verbatim. + * + * @param validationQuery the query used for validation, or null or a blank + * string if no validation query is set + */ @Override public void setValidationQuery(String validationQuery) { - this.validationQuery = validationQuery; + this.validationQuery = validationQuery != null && validationQuery.trim().length() > 0 ? validationQuery : null; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index 748ab20f76..e9929c416b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -924,7 +924,7 @@ public class PooledConnection implements PooledConnectionMBean { } } - String query = sql; + String query = executableStatement(sql); // The init SQL is not a health check: it is the pool deliberately // initializing the physical session it has just established, and its @@ -940,7 +940,7 @@ public class PooledConnection implements PooledConnectionMBean { } if (query == null) { - query = poolProperties.getValidationQuery(); + query = executableStatement(poolProperties.getValidationQuery()); } if (query == null) { @@ -1053,6 +1053,31 @@ public class PooledConnection implements PooledConnectionMBean { } + /** + * Returns the statement a validation should execute, or {@code null} when + * it has none and the built-in {@link java.sql.Connection#isValid(int)} + * check runs instead. + * + * <p>A blank string is not a statement. A configuration surface that + * cannot express the unset value - a JNDI attribute, a connection of the + * two-argument {@link #validate(int, String)} - is written blank to mean + * exactly what the unset value means, so a blank is read as unset here as + * well. {@link PoolProperties#setValidationQuery(String)} already collapses + * a blank at the write, so what reaches this point blank is a configuration + * that did not pass through that setter - one handed in as another + * {@link PoolConfiguration} implementation - or a caller's explicit + * argument, and neither should have an emptiness executed as SQL.</p> + * + * @param query the configured or passed statement, may be {@code null} + * + * @return the query itself if it carries anything, {@code null} if it is + * null or blank + */ + private static String executableStatement(String query) { + return query == null || query.trim().isEmpty() ? null : query; + } + + /** * Terminates a transaction open on the session by committing it, for the * one case where the pending transaction is the pool's own deliberate diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java new file mode 100644 index 0000000000..db50a9efcc --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java @@ -0,0 +1,237 @@ +/* + * 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.tomcat.jdbc.pool; + +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * A blank validation query is the unset value, not a statement. Configuration + * surfaces that cannot express {@code null} - a JNDI attribute, a + * connection-properties line - are routinely written with an empty + * {@code validationQuery} to mean "no query, run {@link Connection#isValid(int)} + * instead", and an emptiness executed as SQL is at best a wasted round trip + * and, on a driver that refuses empty statements, a validation that fails + * every connection the pool touches. The setter collapses the blank the same + * way {@link PoolProperties#setInitSQL(String)} does, and the validation path + * reads a blank as unset even when it reaches the configuration around that + * setter - through a {@link PoolConfiguration} implementation of the pool's + * own, or as the explicit statement argument of + * {@link PooledConnection#validate(int, String)}. + * + * <p>The path witness is a recording {@link Connection}: the built-in check + * goes through {@code isValid()} and the statement path goes through + * {@code createStatement()}, so which half of the contract a case took is + * counted, not guessed at through a driver's tolerance of the emptiness.</p> + */ +class TestBlankValidationQuery { + + /** Counts which half of the validation contract a connect's validation ran through. */ + private static final class Recording { + int isValidCalls; + int createStatementCalls; + int executeCalls; + String lastExecuted; + } + + private static Connection recordingConnection(Recording rec) { + return (Connection) Proxy.newProxyInstance(TestBlankValidationQuery.class.getClassLoader(), + new Class<?>[] { Connection.class }, (proxy, method, args) -> { + switch (method.getName()) { + case "isValid" : + rec.isValidCalls++; + return Boolean.TRUE; + case "createStatement" : + rec.createStatementCalls++; + return recordingStatement(rec); + case "getAutoCommit" : + return Boolean.TRUE; + case "isClosed" : + return Boolean.FALSE; + case "equals" : + return proxy == args[0]; + case "hashCode" : + return Integer.valueOf(System.identityHashCode(proxy)); + case "toString" : + return "recording-connection"; + default : + return defaultValue(method.getReturnType()); + } + }); + } + + private static Statement recordingStatement(Recording rec) { + return (Statement) Proxy.newProxyInstance(TestBlankValidationQuery.class.getClassLoader(), + new Class<?>[] { Statement.class }, (proxy, method, args) -> { + switch (method.getName()) { + case "execute" : + rec.executeCalls++; + rec.lastExecuted = args == null || args.length == 0 ? null : (String) args[0]; + return Boolean.TRUE; + case "equals" : + return proxy == args[0]; + case "hashCode" : + return Integer.valueOf(System.identityHashCode(proxy)); + case "toString" : + return "recording-statement"; + default : + return defaultValue(method.getReturnType()); + } + }); + } + + private static Object defaultValue(Class<?> type) { + if (type == boolean.class || type == Boolean.class) { + return Boolean.FALSE; + } + if (type == int.class || type == Integer.class) { + return Integer.valueOf(0); + } + if (type == long.class || type == Long.class) { + return Long.valueOf(0); + } + if (type == short.class || type == Short.class) { + return Short.valueOf((short) 0); + } + if (type == byte.class || type == Byte.class) { + return Byte.valueOf((byte) 0); + } + if (type == char.class || type == Character.class) { + return Character.valueOf((char) 0); + } + if (type == double.class || type == Double.class) { + return Double.valueOf(0); + } + if (type == float.class || type == Float.class) { + return Float.valueOf(0); + } + return null; + } + + /** A configuration handing out a query that never passed through the collapsing setter. */ + private static final class BypassedConfiguration extends PoolProperties { + private final String rawQuery; + + BypassedConfiguration(String rawQuery) { + this.rawQuery = rawQuery; + } + + @Override + public String getValidationQuery() { + return rawQuery; + } + } + + /** + * A connection that is up against a stand-in {@link javax.sql.DataSource} + * handing out the recording connection - the connect genuinely runs, but + * every call the validation makes lands in the recorder. + */ + private static PooledConnection connectedTo(PoolProperties props, Recording rec) throws Exception { + props.setDataSource(Proxy.newProxyInstance(TestBlankValidationQuery.class.getClassLoader(), + new Class<?>[] { javax.sql.DataSource.class }, (proxy, method, args) -> { + switch (method.getName()) { + case "getConnection" : + return recordingConnection(rec); + case "equals" : + return proxy == args[0]; + case "hashCode" : + return Integer.valueOf(System.identityHashCode(proxy)); + case "toString" : + return "recording-datasource"; + default : + return defaultValue(method.getReturnType()); + } + })); + props.setJmxEnabled(false); + props.setInitialSize(0); + props.setTimeBetweenEvictionRunsMillis(-1); + // Without this the interval suppression - by design - answers "true" + // for the first seconds after construction and the validation never + // reaches the calls this recorder watches. + props.setValidationInterval(0); + props.setTestOnBorrow(true); + PooledConnection con = new PooledConnection(props, new ConnectionPool(props)); + con.connect(); + return con; + } + + @ParameterizedTest(name = "configured query [{0}] validates through isValid()") + @ValueSource(strings = { "", " ", "\t" }) + void aBlankQueryReachingTheValidationAroundTheSetterIsNotExecuted(String blank) throws Exception { + Recording rec = new Recording(); + // A PoolConfiguration implementation of the caller's own can hand out a + // blank the pool's setter would never have stored. + PooledConnection con = connectedTo(new BypassedConfiguration(blank), rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_BORROW), + "a blank query means the built-in check, which the session passes"); + Assertions.assertEquals(1, rec.isValidCalls, "the built-in check is what must have run"); + Assertions.assertEquals(0, rec.createStatementCalls, + "a blank must never be prepared and executed as a statement"); + } + + @Test + void anExplicitBlankArgumentMeansNoArgument() throws Exception { + Recording rec = new Recording(); + PoolProperties props = new PoolProperties(); + props.setValidationQuery("SELECT 1"); + PooledConnection con = connectedTo(props, rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_BORROW, "")); + Assertions.assertEquals(1, rec.executeCalls, "the configured query still validates"); + Assertions.assertEquals("SELECT 1", rec.lastExecuted, + "an empty argument reads as no argument, so it is the configured query that runs"); + Assertions.assertEquals(0, rec.isValidCalls); + } + + @Test + void anExplicitQueryArgumentIsStillExecuted() throws Exception { + Recording rec = new Recording(); + PooledConnection con = connectedTo(new PoolProperties(), rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_BORROW, "SELECT 42")); + Assertions.assertEquals(1, rec.executeCalls); + Assertions.assertEquals("SELECT 42", rec.lastExecuted); + Assertions.assertEquals(0, rec.isValidCalls, + "a caller that names a statement gets that statement, not the built-in check"); + } + + @ParameterizedTest(name = "setter collapses [{0}] to unset") + @ValueSource(strings = { "", " ", "\t" }) + void theSetterStoresABlankQueryAsTheUnsetValue(String blank) { + PoolProperties props = new PoolProperties(); + props.setValidationQuery("SELECT 1"); + props.setValidationQuery(blank); + Assertions.assertNull(props.getValidationQuery(), + "the blank must read back as the unset value, the way setInitSQL collapses it"); + } + + @Test + void theSetterKeepsAConfiguredQueryVerbatim() { + PoolProperties props = new PoolProperties(); + props.setValidationQuery(" SELECT 1 FROM DUAL "); + Assertions.assertEquals(" SELECT 1 FROM DUAL ", props.getValidationQuery(), + "a query that carries anything is stored exactly as written"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
