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 ff8cfb7779cdb8db73bcc92f7cb8f6090a07a524 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 09:50:09 2026 +0100 Do not let query reporting replace the exception a statement method raised The statement proxy installed by the query report interceptors reported a failure for every statement method that threw, not only for executions. The base report hook extracted the SQL with an unchecked (String) args[0] when the statement had been created through createStatement(), so a failing non-query call whose first argument was not a string - setFetchSize(-1), setQueryTimeout(-1), getMoreResults(int) - threw a ClassCastException from inside the catch block. That exception escape [...] Report failures only for the methods the proxy recognises as executions, which is what the hooks' javadoc already documented, and extract the SQL only when the argument really is a string. --- .../jdbc/pool/interceptor/AbstractQueryReport.java | 32 +++++++-- .../pool/interceptor/TestInterceptorSemantics.java | 66 +++++++++++++++++++ .../interceptor/TestQueryReportExtraction.java | 77 ++++++++++++++++++++++ 3 files changed, 168 insertions(+), 7 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java index 1171e65a91..28dad3c3b4 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java @@ -68,11 +68,15 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce * @param name the name of the method used to execute {@link AbstractCreateStatementInterceptor#isExecute(Method, boolean)} * @param start the time the query execution started * @param t the exception that happened - * @return - the SQL that was executed or the string "batch" if it was a batch execution + * @return - the SQL that was executed or the string "batch" if it was a batch execution, + * or {@code null} if the invocation carries no SQL string */ protected String reportFailedQuery(String query, Object[] args, final String name, long start, Throwable t) { //extract the query string - String sql = (query==null && args!=null && args.length>0)?(String)args[0]:query; + String sql = query; + if (sql == null && args != null && args.length > 0 && args[0] instanceof String) { + sql = (String) args[0]; + } //if we do batch execution, then we name the query 'batch' if (sql==null && compare(EXECUTE_BATCH,name)) { sql = "batch"; @@ -87,11 +91,15 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce * @param name the name of the method used to execute {@link AbstractCreateStatementInterceptor#isExecute(Method, boolean)} * @param start the time the query execution started * @param delta the time the execution took - * @return - the SQL that was executed or the string "batch" if it was a batch execution + * @return - the SQL that was executed or the string "batch" if it was a batch execution, + * or {@code null} if the invocation carries no SQL string */ protected String reportQuery(String query, Object[] args, final String name, long start, long delta) { //extract the query string - String sql = (query==null && args!=null && args.length>0)?(String)args[0]:query; + String sql = query; + if (sql == null && args != null && args.length > 0 && args[0] instanceof String) { + sql = (String) args[0]; + } //if we do batch execution, then we name the query 'batch' if (sql==null && compare(EXECUTE_BATCH,name)) { sql = "batch"; @@ -106,11 +114,15 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce * @param name the name of the method used to execute {@link AbstractCreateStatementInterceptor#isExecute(Method, boolean)} * @param start the time the query execution started * @param delta the time the execution took - * @return - the SQL that was executed or the string "batch" if it was a batch execution + * @return - the SQL that was executed or the string "batch" if it was a batch execution, + * or {@code null} if the invocation carries no SQL string */ protected String reportSlowQuery(String query, Object[] args, final String name, long start, long delta) { //extract the query string - String sql = (query==null && args!=null && args.length>0)?(String)args[0]:query; + String sql = query; + if (sql == null && args != null && args.length > 0 && args[0] instanceof String) { + sql = (String) args[0]; + } //if we do batch execution, then we name the query 'batch' if (sql==null && compare(EXECUTE_BATCH,name)) { sql = "batch"; @@ -231,7 +243,13 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce //execute the query result = method.invoke(delegate,args); }catch (Throwable t) { - reportFailedQuery(query,args,name,start,t); + //only report the failure if we were executing a query. + //For any other method there is no query to report, and + //reporting it would replace the exception that the + //application is expecting with a reporting failure + if (process) { + reportFailedQuery(query,args,name,start,t); + } if (t instanceof InvocationTargetException && t.getCause() != null) { throw t.getCause(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java index 13dc82911c..d5a4c2e381 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorSemantics.java @@ -87,6 +87,72 @@ class TestInterceptorSemantics extends JdbcPoolBaseTest { + ", recorded keys: " + statsKeySet(stats)); } + /* + * A failing statement method that is not an execution has no query to + * report. Reporting it anyway made the reporting code itself fail - the + * query extraction cast the int argument of setFetchSize to String - + * and the ClassCastException escaped in place of the SQLException the + * driver raised, which no application catching SQLException expects. + */ + @Test + void failedNonExecuteMethodPropagatesTheDriverException() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport"); + ds.createPool(); + + try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { + assertThrows(SQLException.class, () -> st.setFetchSize(-1), + "the driver's SQLException must reach the application"); + } + } + + /* + * Same for a prepared statement, where the extraction succeeds: the + * failure used to be booked against the statement's SQL with a start + * time of 0, so the recorded "execution time" was the epoch. + */ + @Test + void failedNonExecuteMethodIsNotBookedAsAFailedQuery() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport"); + ds.createPool(); + String poolName = ds.getPoolProperties().getPoolName(); + + String nonce = "nontime_" + System.nanoTime(); + String sql = "SELECT 1 /* " + nonce + " */"; + Connection c = ds.getConnection(); + PreparedStatement ps = c.prepareStatement(sql); + // a negative fetch size is a SQLException per the JDBC contract + assertThrows(SQLException.class, () -> ps.setFetchSize(-1)); + + // preparing the statement legitimately creates the stats entry, + // but a method that is not an execution must not be booked in it + ConcurrentHashMap<String, ?> stats = SlowQueryReport.getPoolStats(poolName); + assertNotNull(stats, "the prepare must have created the stats map"); + String key = statsKeySet(stats).stream().filter(k -> k.contains(nonce)) + .findFirst().orElse(null); + assertNotNull(key, "the prepared statement must have a stats entry, " + + "recorded keys: " + statsKeySet(stats)); + SlowQueryReport.QueryStats qs = (SlowQueryReport.QueryStats) stats.get(key); + assertEquals(0, qs.getNrOfInvocations(), + "a method that is not an execution must not be booked as an " + + "invocation of " + key); + assertEquals(Long.MIN_VALUE, qs.getMaxInvocationTime(), + "no invocation time may be recorded for a method that is not " + + "an execution"); + + // positive control: the report must still be live for this statement, + // so the assertions above cannot pass because the proxy is absent + ps.execute(); + assertEquals(1, qs.getNrOfInvocations(), + "the execution must be booked in the same entry"); + + ps.close(); + c.close(); + } + private static java.util.Set<String> statsKeySet(ConcurrentHashMap<String, ?> stats) { return stats.keySet(); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportExtraction.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportExtraction.java new file mode 100644 index 0000000000..af54fa88f3 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportExtraction.java @@ -0,0 +1,77 @@ +/* + * 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.interceptor; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import org.junit.jupiter.api.Test; + +/** + * The SQL extraction the query report hooks perform, checked in + * isolation. The SQL a statement was prepared with takes precedence over + * the argument of the invoked method, an argument that is not a string is + * not a query - it must not abort the report - and a batch executed on a + * plain statement is reported under the name {@code batch}. + */ +class TestQueryReportExtraction { + + private static final AbstractQueryReport REPORT = new AbstractQueryReport() { + @Override + protected void prepareStatement(String sql, long time) { + // extraction only - nothing to record + } + + @Override + protected void prepareCall(String query, long time) { + // extraction only - nothing to record + } + + @Override + public void closeInvoked() { + // extraction only - nothing to record + } + }; + + @Test + void aNonStringArgumentIsNotAQuery() { + // the argument of setFetchSize(int) and friends is not SQL + assertNull(REPORT.reportFailedQuery(null, new Object[] { Integer.valueOf(5) }, + "setFetchSize", 0, null)); + // but the SQL of the prepared statement still is + assertEquals("SELECT 1", REPORT.reportFailedQuery("SELECT 1", + new Object[] { Integer.valueOf(5) }, "setFetchSize", 0, null)); + } + + @Test + void aBatchOnAPlainStatementIsNamedBatch() { + // a batch execution carries no SQL and no arguments at all + assertEquals("batch", + REPORT.reportFailedQuery(null, null, "executeBatch", 0, null)); + assertEquals("batch", + REPORT.reportQuery(null, new Object[0], "executeBatch", 0, 1)); + assertEquals("batch", + REPORT.reportSlowQuery(null, null, "executeBatch", 0, 1)); + } + + @Test + void theSQLOfTheStatementWinsOverTheMethodArgument() { + assertEquals("CALL p()", REPORT.reportQuery("CALL p()", + new Object[] { "SELECT 1" }, "execute", 0, 1)); + assertEquals("SELECT 1", REPORT.reportSlowQuery(null, + new Object[] { "SELECT 1" }, "executeQuery", 0, 1)); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
