From 4ea6b4cb148bf90ea5b052c009f59e5034614d15 Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 29 Apr 2025 22:21:52 -0400 Subject: [PATCH 1/6] Fixed multiple connection leaks in DataSourceResourceLoader. The first case was the implementation of FilterReader would get the connection from a result set after closing the result set. This always throws an exception as it is an invalid operation after closing the result set. The second is in the getResourceReader method. In the happy path a FilterReader is returned which eventually closes the connection when close is called. However, if a template is not found, an exception is thrown without closing the connection. --- .../loader/DataSourceResourceLoader.java | 15 ++++++-- .../sql/DataSourceResourceLoaderTestCase.java | 34 +++++++++++++++++-- .../TestDefaultDatabaseObjectsFactory.java | 27 +++++++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java index 21e1a1e0a..63a39dc34 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java @@ -165,8 +165,12 @@ public void close() throws IOException super.close(); try { - resultSet.close(); - factory.releaseStatement(templateSQL, (PreparedStatement)resultSet.getStatement()); + PreparedStatement statement = (PreparedStatement) resultSet.getStatement(); + try { + resultSet.close(); + } finally { + factory.releaseStatement(templateSQL, statement); + } } catch (RuntimeException re) { @@ -295,7 +299,7 @@ public synchronized Reader getResourceReader(final String name, String encoding) throw new ResourceNotFoundException("DataSourceResourceLoader: Template name was empty or null"); } - ResultSet rs = null; + ResultSet rs; try { PreparedStatement statement = factory.prepareStatement(templateSQL); @@ -314,6 +318,11 @@ public synchronized Reader getResourceReader(final String name, String encoding) } else { + try { + closeResultSet(rs); + } finally { + factory.releaseStatement(templateSQL, statement); + } throw new ResourceNotFoundException("DataSourceResourceLoader: " + "could not find resource '" + name + "'"); diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java index d4333b73a..acc6ab3e1 100644 --- a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java +++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java @@ -69,6 +69,9 @@ public class DataSourceResourceLoaderTestCase /* engine with VARCHAR templates data source */ private RuntimeInstance varcharTemplatesEngine = null; + /* engine with VARCHAR templates data source for testing connection counts*/ + private RuntimeInstance varcharTemplatesConnectionCountTestEngine = null; + /* engine with CLOB templates data source */ private RuntimeInstance clobTemplatesEngine = null; @@ -98,6 +101,10 @@ public void setUp() DataSourceResourceLoader rl2 = new DataSourceResourceLoader(); rl2.setDataSource(ds2); + DataSource ds3 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD); + DataSourceResourceLoader rl3 = new DataSourceResourceLoader(); + rl3.setDataSource(ds3); + ExtProperties props = getResourceLoaderProperties(); props.setProperty( "ds.resource.loader.instance", rl1); props.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); @@ -112,6 +119,14 @@ public void setUp() clobTemplatesEngine = new RuntimeInstance(); clobTemplatesEngine.setConfiguration(props2); clobTemplatesEngine.init(); + + ExtProperties props3 = getResourceLoaderProperties(); + props3.setProperty( "ds.resource.loader.instance", rl3); + props3.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); + props3.setProperty( "ds.resource.loader.database_objects_factory.class", "org.apache.velocity.test.sql.TestDefaultDatabaseObjectsFactory"); + varcharTemplatesConnectionCountTestEngine = new RuntimeInstance(); + varcharTemplatesConnectionCountTestEngine.setConfiguration(props3); + varcharTemplatesConnectionCountTestEngine.init(); } protected ExtProperties getResourceLoaderProperties() @@ -129,13 +144,24 @@ protected ExtProperties getResourceLoaderProperties() * Tests loading and rendering of a simple template. If that works, we are able to get data * from the database. */ + public void testSimpleTemplate() throws Exception { Template t = executeTest("testTemplate1", varcharTemplatesEngine); assertFalse("Timestamp is 0", 0 == t.getLastModified()); t = executeTest("testTemplate1", clobTemplatesEngine); - assertFalse("Timestamp is 0", 0 == t.getLastModified()); } + assertFalse("Timestamp is 0", 0 == t.getLastModified()); + } + /** + * Tests loading and rendering of a simple template and checks that there are no connection leaks. + */ + public void testForConnectionLeaks() + throws Exception + { + executeTest("testTemplate1", varcharTemplatesConnectionCountTestEngine); + assertEquals("Open connection count is greater then 0", 0, TestDefaultDatabaseObjectsFactory.getConnectionCount()); + } public void testUnicode(RuntimeInstance engine) throws Exception @@ -161,6 +187,7 @@ public void testUnicode(RuntimeInstance engine) * Now we have a more complex example. Run a very simple tool. * from the database. */ + public void testRenderTool() throws Exception { @@ -173,17 +200,20 @@ public void testRenderTool() /** * Will a NULL timestamp choke the loader? */ + public void testNullTimestamp() throws Exception { Template t = executeTest("testTemplate3", varcharTemplatesEngine); assertEquals("Timestamp is not 0", 0, t.getLastModified()); t = executeTest("testTemplate3", clobTemplatesEngine); - assertEquals("Timestamp is not 0", 0, t.getLastModified()); } + assertEquals("Timestamp is not 0", 0, t.getLastModified()); + } /** * Does it load the global Macros from the DB? */ + public void testMacroInvocation() throws Exception { diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java new file mode 100644 index 000000000..10a2f805b --- /dev/null +++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java @@ -0,0 +1,27 @@ +package org.apache.velocity.test.sql; + +import org.apache.velocity.runtime.resource.loader.DefaultDatabaseObjectsFactory; + +import java.sql.PreparedStatement; +import java.sql.SQLException; + +public class TestDefaultDatabaseObjectsFactory extends DefaultDatabaseObjectsFactory { + public static int connectionCount = 0; + + @Override + public PreparedStatement prepareStatement(String sql) throws SQLException { + PreparedStatement ps = super.prepareStatement(sql); + connectionCount++; + return ps; + } + + @Override + public void releaseStatement(String sql, PreparedStatement stmt) throws SQLException { + + super.releaseStatement(sql, stmt); + connectionCount--; + } + public static int getConnectionCount() { + return connectionCount; + } +} From 81f4aec2e7c52828b397f2746eb72f3a4f33a029 Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 29 Apr 2025 22:51:41 -0400 Subject: [PATCH 2/6] Added explicit test for not finding template. --- .../test/sql/DataSourceResourceLoaderTestCase.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java index acc6ab3e1..9e00edd38 100644 --- a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java +++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java @@ -24,6 +24,7 @@ import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; import org.apache.velocity.app.Velocity; +import org.apache.velocity.exception.ResourceNotFoundException; import org.apache.velocity.runtime.RuntimeInstance; import org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader; import org.apache.velocity.test.misc.TestLogger; @@ -160,6 +161,12 @@ public void testForConnectionLeaks() throws Exception { executeTest("testTemplate1", varcharTemplatesConnectionCountTestEngine); + try { + varcharTemplatesConnectionCountTestEngine.getTemplate("fakeTemplate"); + fail("Should have thrown exception ResourceNotFoundException"); + } catch (ResourceNotFoundException e) { + //continue + } assertEquals("Open connection count is greater then 0", 0, TestDefaultDatabaseObjectsFactory.getConnectionCount()); } From d9395149de0225613eb832659b325977b518e089 Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 13 May 2025 00:32:29 -0400 Subject: [PATCH 3/6] Added proxy for PreparedStatement and ResultSet that guarantees PreparedStatement.getConnection and ResultSet.getStatement will return the wrapped objects created by the underlying DataSource and not return null or throw an exception when closed. Removed clumsy TestDefaultDatabaseObjectsFactory.java that was used to test for connection leaks and instead added 3 new tests that use commonly pooled DataSource implementations. --- velocity-engine-core/pom.xml | 17 +++ .../loader/DataSourceResourceLoader.java | 8 +- .../loader/DefaultDatabaseObjectsFactory.java | 10 +- .../loader/PreparedStatementProxy.java | 64 ++++++++++ .../resource/loader/ResultSetProxy.java | 54 ++++++++ .../sql/DataSourceResourceLoaderTestCase.java | 119 ++++++++++++++---- .../TestDefaultDatabaseObjectsFactory.java | 27 ---- 7 files changed, 246 insertions(+), 53 deletions(-) create mode 100644 velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java create mode 100644 velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java delete mode 100644 velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java diff --git a/velocity-engine-core/pom.xml b/velocity-engine-core/pom.xml index 27e705910..afab46bcb 100644 --- a/velocity-engine-core/pom.xml +++ b/velocity-engine-core/pom.xml @@ -252,6 +252,23 @@ ${test.jdbc.driver.classifier} + org.apache.commons + commons-dbcp2 + 2.13.0 + test + + + org.apache.tomcat + tomcat-jdbc + 9.0.104 + test + + + com.zaxxer + HikariCP + 4.0.3 + test + org.slf4j slf4j-simple test diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java index 63a39dc34..b7b75963d 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java @@ -167,7 +167,9 @@ public void close() throws IOException { PreparedStatement statement = (PreparedStatement) resultSet.getStatement(); try { - resultSet.close(); + if (!resultSet.isClosed()) { + resultSet.close(); + } } finally { factory.releaseStatement(templateSQL, statement); } @@ -417,7 +419,9 @@ private void closeResultSet(final ResultSet rs) { try { - rs.close(); + if (!rs.isClosed()) { + rs.close(); + } } catch (RuntimeException re) { diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java index 6edc61245..91384a028 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java @@ -35,7 +35,7 @@ public void init(DataSource dataSource, ExtProperties properties) public PreparedStatement prepareStatement(String sql) throws SQLException { Connection connection = dataSource.getConnection(); - return connection.prepareStatement(sql); + return PreparedStatementProxy.newInstance(connection.prepareStatement(sql), connection); } /** @@ -49,11 +49,15 @@ public void releaseStatement(String sql, PreparedStatement stmt) throws SQLExcep Connection connection = stmt.getConnection(); try { - stmt.close(); + if (!stmt.isClosed()) { + stmt.close(); + } } finally { - connection.close(); + if (!connection.isClosed()) { + connection.close(); + } } } } diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java new file mode 100644 index 000000000..092009b68 --- /dev/null +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java @@ -0,0 +1,64 @@ +package org.apache.velocity.runtime.resource.loader; +/* + * 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. + */ + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; + +public class PreparedStatementProxy implements InvocationHandler { + private final PreparedStatement wrappedPreparedStatement; + private PreparedStatement proxyPreparedStatement; + private final Connection wrappedConnection; + + public PreparedStatementProxy(PreparedStatement wrappedPreparedStatement, Connection wrappedConnection) { + this.wrappedPreparedStatement = wrappedPreparedStatement; + this.wrappedConnection = wrappedConnection; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + Object result; + if (method.getName().equals("getConnection")) { + result = wrappedConnection; + } else if (method.getName().equals("executeQuery")) { + return ResultSetProxy.newInstance((ResultSet) method.invoke(wrappedPreparedStatement, args), proxyPreparedStatement); + } else { + result = method.invoke(wrappedPreparedStatement, args); + } + return result; + } + + public static PreparedStatement newInstance(PreparedStatement ps, Connection con) throws SQLException { + PreparedStatementProxy invocationHandler = new PreparedStatementProxy(ps, con); + PreparedStatement proxyPreparedStatement = (PreparedStatement) Proxy.newProxyInstance(ps.getClass().getClassLoader(), + ps.getClass().getInterfaces(), + invocationHandler); + invocationHandler.setProxyPreparedStatement(proxyPreparedStatement); + return proxyPreparedStatement; + } + + public void setProxyPreparedStatement(PreparedStatement proxyPreparedStatement) { + this.proxyPreparedStatement = proxyPreparedStatement; + } +} diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java new file mode 100644 index 000000000..c59d92329 --- /dev/null +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java @@ -0,0 +1,54 @@ +package org.apache.velocity.runtime.resource.loader; +/* + * 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. + */ + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; + +public class ResultSetProxy implements InvocationHandler { + private final ResultSet wrappedResultSet; + private final PreparedStatement wrappedPreparedStatement; + + public ResultSetProxy(ResultSet wrappedResultSet, PreparedStatement wrappedPreparedStatement) { + this.wrappedResultSet = wrappedResultSet; + this.wrappedPreparedStatement = wrappedPreparedStatement; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + Object result; + if (method.getName().equals("getStatement")) { + result = wrappedPreparedStatement; + } else { + result = method.invoke(wrappedResultSet, args); + } + return result; + } + + public static ResultSet newInstance(ResultSet rs, PreparedStatement ps) throws SQLException { + return (ResultSet) Proxy.newProxyInstance(rs.getClass().getClassLoader(), + rs.getClass().getInterfaces(), + new ResultSetProxy(rs, ps)); + } + +} diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java index 9e00edd38..dd0b9eae9 100644 --- a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java +++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java @@ -19,8 +19,10 @@ * under the License. */ +import com.zaxxer.hikari.HikariDataSource; import junit.framework.Test; import junit.framework.TestSuite; +import org.apache.commons.dbcp2.BasicDataSource; import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; import org.apache.velocity.app.Velocity; @@ -70,8 +72,17 @@ public class DataSourceResourceLoaderTestCase /* engine with VARCHAR templates data source */ private RuntimeInstance varcharTemplatesEngine = null; - /* engine with VARCHAR templates data source for testing connection counts*/ - private RuntimeInstance varcharTemplatesConnectionCountTestEngine = null; + /* engine with VARCHAR templates data source for testing connection counts with DBCP2 data source*/ + private RuntimeInstance varcharTemplatesDBCP2ConnectionCountTestEngine = null; + private BasicDataSource dbcp2ConnectionCountDataSource = null; + + /* engine with VARCHAR templates data source for testing connection counts with Tomcat JDBC data source*/ + private RuntimeInstance varcharTemplatesTomcatJDBCConnectionCountTestEngine = null; + private org.apache.tomcat.jdbc.pool.DataSource tomcatJDBCConnectionCountDataSource = null; + + /* engine with VARCHAR templates data source for testing connection counts with Tomcat JDBC data source*/ + private RuntimeInstance varcharTemplatesHikariConnectionCountTestEngine = null; + private HikariDataSource hikariConnectionCountDataSource = null; /* engine with CLOB templates data source */ private RuntimeInstance clobTemplatesEngine = null; @@ -97,23 +108,16 @@ public void setUp() DataSource ds1 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD); DataSourceResourceLoader rl1 = new DataSourceResourceLoader(); rl1.setDataSource(ds1); - - DataSource ds2 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD); - DataSourceResourceLoader rl2 = new DataSourceResourceLoader(); - rl2.setDataSource(ds2); - - DataSource ds3 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD); - DataSourceResourceLoader rl3 = new DataSourceResourceLoader(); - rl3.setDataSource(ds3); - ExtProperties props = getResourceLoaderProperties(); props.setProperty( "ds.resource.loader.instance", rl1); props.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); - varcharTemplatesEngine = new RuntimeInstance(); varcharTemplatesEngine.setConfiguration(props); varcharTemplatesEngine.init(); + DataSource ds2 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD); + DataSourceResourceLoader rl2 = new DataSourceResourceLoader(); + rl2.setDataSource(ds2); ExtProperties props2 = (ExtProperties)props.clone(); props2.setProperty( "ds.resource.loader.instance", rl2); props2.setProperty( "ds.resource.loader.resource.table", "velocity_template_clob"); @@ -121,13 +125,54 @@ public void setUp() clobTemplatesEngine.setConfiguration(props2); clobTemplatesEngine.init(); + BasicDataSource ds3 = new BasicDataSource(); + ds3.setDriverClassName(TEST_JDBC_DRIVER_CLASS); + ds3.setUrl(TEST_JDBC_URI); + ds3.setUsername(TEST_JDBC_LOGIN); + ds3.setPassword(TEST_JDBC_PASSWORD); + ds3.setMaxTotal(10); + DataSourceResourceLoader rl3 = new DataSourceResourceLoader(); + rl3.setDataSource(ds3); ExtProperties props3 = getResourceLoaderProperties(); props3.setProperty( "ds.resource.loader.instance", rl3); props3.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); - props3.setProperty( "ds.resource.loader.database_objects_factory.class", "org.apache.velocity.test.sql.TestDefaultDatabaseObjectsFactory"); - varcharTemplatesConnectionCountTestEngine = new RuntimeInstance(); - varcharTemplatesConnectionCountTestEngine.setConfiguration(props3); - varcharTemplatesConnectionCountTestEngine.init(); + varcharTemplatesDBCP2ConnectionCountTestEngine = new RuntimeInstance(); + varcharTemplatesDBCP2ConnectionCountTestEngine.setConfiguration(props3); + varcharTemplatesDBCP2ConnectionCountTestEngine.init(); + dbcp2ConnectionCountDataSource = ds3; + + org.apache.tomcat.jdbc.pool.DataSource ds4 = new org.apache.tomcat.jdbc.pool.DataSource(); + ds4.setDriverClassName(TEST_JDBC_DRIVER_CLASS); + ds4.setUrl(TEST_JDBC_URI); + ds4.setUsername(TEST_JDBC_LOGIN); + ds4.setPassword(TEST_JDBC_PASSWORD); + ds4.setMaxActive(10); + DataSourceResourceLoader rl4 = new DataSourceResourceLoader(); + rl4.setDataSource(ds4); + ExtProperties props4 = getResourceLoaderProperties(); + props4.setProperty( "ds.resource.loader.instance", rl4); + props4.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); + varcharTemplatesTomcatJDBCConnectionCountTestEngine = new RuntimeInstance(); + varcharTemplatesTomcatJDBCConnectionCountTestEngine.setConfiguration(props4); + varcharTemplatesTomcatJDBCConnectionCountTestEngine.init(); + tomcatJDBCConnectionCountDataSource = ds4; + + HikariDataSource ds5 = new HikariDataSource(); + ds5.setDriverClassName(TEST_JDBC_DRIVER_CLASS); + ds5.setJdbcUrl(TEST_JDBC_URI); + ds5.setUsername(TEST_JDBC_LOGIN); + ds5.setPassword(TEST_JDBC_PASSWORD); + ds5.setMaximumPoolSize(10); + DataSourceResourceLoader rl5 = new DataSourceResourceLoader(); + rl5.setDataSource(ds5); + ExtProperties props5 = getResourceLoaderProperties(); + props5.setProperty( "ds.resource.loader.instance", rl5); + props5.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar"); + varcharTemplatesHikariConnectionCountTestEngine = new RuntimeInstance(); + varcharTemplatesHikariConnectionCountTestEngine.setConfiguration(props5); + varcharTemplatesHikariConnectionCountTestEngine.init(); + hikariConnectionCountDataSource = ds5; + } protected ExtProperties getResourceLoaderProperties() @@ -155,19 +200,51 @@ public void testSimpleTemplate() assertFalse("Timestamp is 0", 0 == t.getLastModified()); } /** - * Tests loading and rendering of a simple template and checks that there are no connection leaks. + * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses DBCP2 Data data source + */ + public void testDBCP2DataSourceForConnectionLeaks() + throws Exception + { + executeTest("testTemplate1", varcharTemplatesDBCP2ConnectionCountTestEngine); + try { + varcharTemplatesDBCP2ConnectionCountTestEngine.getTemplate("fakeTemplate"); + fail("Should have thrown exception ResourceNotFoundException"); + } catch (ResourceNotFoundException e) { + //continue + } + assertEquals("Open connection count is greater then 0", 0, this.dbcp2ConnectionCountDataSource.getConnectionPool().getNumActive()); + } + + /** + * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses Tomcat JDBC data source + */ + public void testTomcatJDBCDataSourceForConnectionLeaks() + throws Exception + { + executeTest("testTemplate1", varcharTemplatesTomcatJDBCConnectionCountTestEngine); + try { + varcharTemplatesTomcatJDBCConnectionCountTestEngine.getTemplate("fakeTemplate"); + fail("Should have thrown exception ResourceNotFoundException"); + } catch (ResourceNotFoundException e) { + //continue + } + assertEquals("Open connection count is greater then 0", 0, this.tomcatJDBCConnectionCountDataSource.getActive()); + } + + /** + * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses Tomcat JDBC data source */ - public void testForConnectionLeaks() + public void testHikariCPDataSourceForConnectionLeaks() throws Exception { - executeTest("testTemplate1", varcharTemplatesConnectionCountTestEngine); + executeTest("testTemplate1", varcharTemplatesHikariConnectionCountTestEngine); try { - varcharTemplatesConnectionCountTestEngine.getTemplate("fakeTemplate"); + varcharTemplatesHikariConnectionCountTestEngine.getTemplate("fakeTemplate"); fail("Should have thrown exception ResourceNotFoundException"); } catch (ResourceNotFoundException e) { //continue } - assertEquals("Open connection count is greater then 0", 0, TestDefaultDatabaseObjectsFactory.getConnectionCount()); + assertEquals("Open connection count is greater then 0", 0, this.hikariConnectionCountDataSource.getHikariPoolMXBean().getActiveConnections()); } public void testUnicode(RuntimeInstance engine) diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java deleted file mode 100644 index 10a2f805b..000000000 --- a/velocity-engine-core/src/test/java/org/apache/velocity/test/sql/TestDefaultDatabaseObjectsFactory.java +++ /dev/null @@ -1,27 +0,0 @@ -package org.apache.velocity.test.sql; - -import org.apache.velocity.runtime.resource.loader.DefaultDatabaseObjectsFactory; - -import java.sql.PreparedStatement; -import java.sql.SQLException; - -public class TestDefaultDatabaseObjectsFactory extends DefaultDatabaseObjectsFactory { - public static int connectionCount = 0; - - @Override - public PreparedStatement prepareStatement(String sql) throws SQLException { - PreparedStatement ps = super.prepareStatement(sql); - connectionCount++; - return ps; - } - - @Override - public void releaseStatement(String sql, PreparedStatement stmt) throws SQLException { - - super.releaseStatement(sql, stmt); - connectionCount--; - } - public static int getConnectionCount() { - return connectionCount; - } -} From 197cf4498ea4cf6977065c428fd20ec7aec3cb8f Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 13 May 2025 00:43:14 -0400 Subject: [PATCH 4/6] Added some class comments. --- .../resource/loader/PreparedStatementProxy.java | 5 +++++ .../runtime/resource/loader/ResultSetProxy.java | 13 +++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java index 092009b68..8c114431f 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java @@ -26,6 +26,11 @@ import java.sql.ResultSet; import java.sql.SQLException; +/** + * Proxy for {@link java.sql.PreparedStatement} that guarantees getConnection will always return the DataSource + * provided wrapped {@link java.sql.Connection} and not the underlying database specific connection. + * Also overrides executeQuery to return a proxy for {@link java.sql.ResultSet} see {@link ResultSetProxy} + */ public class PreparedStatementProxy implements InvocationHandler { private final PreparedStatement wrappedPreparedStatement; private PreparedStatement proxyPreparedStatement; diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java index c59d92329..275abc559 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java @@ -24,21 +24,26 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Statement; +/** + * Proxy for {@link java.sql.ResultSet} that guarantees getStatement will return the original wrapped {@link java.sql.Statement} + * and will not throw an exception or return null if the ResultSet is closed. + */ public class ResultSetProxy implements InvocationHandler { private final ResultSet wrappedResultSet; - private final PreparedStatement wrappedPreparedStatement; + private final Statement wrappedStatement; - public ResultSetProxy(ResultSet wrappedResultSet, PreparedStatement wrappedPreparedStatement) { + public ResultSetProxy(ResultSet wrappedResultSet, PreparedStatement wrappedStatement) { this.wrappedResultSet = wrappedResultSet; - this.wrappedPreparedStatement = wrappedPreparedStatement; + this.wrappedStatement = wrappedStatement; } @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { Object result; if (method.getName().equals("getStatement")) { - result = wrappedPreparedStatement; + result = wrappedStatement; } else { result = method.invoke(wrappedResultSet, args); } From 62ceceeb872fbde8ab0a8bdd4d4af61bbaeda40c Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 13 May 2025 01:01:34 -0400 Subject: [PATCH 5/6] type consistency. --- .../velocity/runtime/resource/loader/ResultSetProxy.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java index 275abc559..7f068c005 100644 --- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java +++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java @@ -21,7 +21,6 @@ import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.lang.reflect.Proxy; -import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; @@ -34,7 +33,7 @@ public class ResultSetProxy implements InvocationHandler { private final ResultSet wrappedResultSet; private final Statement wrappedStatement; - public ResultSetProxy(ResultSet wrappedResultSet, PreparedStatement wrappedStatement) { + public ResultSetProxy(ResultSet wrappedResultSet, Statement wrappedStatement) { this.wrappedResultSet = wrappedResultSet; this.wrappedStatement = wrappedStatement; } @@ -50,7 +49,7 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl return result; } - public static ResultSet newInstance(ResultSet rs, PreparedStatement ps) throws SQLException { + public static ResultSet newInstance(ResultSet rs, Statement ps) throws SQLException { return (ResultSet) Proxy.newProxyInstance(rs.getClass().getClassLoader(), rs.getClass().getInterfaces(), new ResultSetProxy(rs, ps)); From 2e95f709fc4bbe7d8d5ceb47108c369775918d1c Mon Sep 17 00:00:00 2001 From: Francis Schlimmer Date: Tue, 13 May 2025 09:05:21 -0400 Subject: [PATCH 6/6] Formatting. --- velocity-engine-core/pom.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/velocity-engine-core/pom.xml b/velocity-engine-core/pom.xml index afab46bcb..8f57641d9 100644 --- a/velocity-engine-core/pom.xml +++ b/velocity-engine-core/pom.xml @@ -268,7 +268,8 @@ HikariCP 4.0.3 test - + + org.slf4j slf4j-simple test