This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 4382167c0318f458e28752cf32085106b3dbcbe1 Author: opencode <[email protected]> AuthorDate: Thu Oct 29 22:10:21 2026 +0100 Accept live data sources and skip empty attributes in the pool JNDI factory getObjectInstance() stringified the content of every RefAddr it looked up, so an attribute carrying null content - which the RefAddr contract permits - aborted the whole factory call with a bare NullPointerException, and a Reference carrying a live javax.sql.DataSource under the dataSource attribute was stringified and then rejected with a message contradicting the documented property type. Attributes with null content are now skipped and an object-valued dataSource attribute is applied to the pool configuration before the dataSourceJNDI lookup, so the pool is backed by the exact object handed to the factory; the existing guard gives it precedence over dataSourceJNDI. A genuinely string value for that attribute is still rejected, with a message that names the supported ways of providing a data source instead of claiming the case should never happen. To keep subclasses that customise the public three argument createDataSource() in the loop, that hook is still dispatched for references without a live data source object; only the object-valued case uses the new protected four argument overload. Adds TestDataSourceFactoryRefAddrContent covering the null-content skip, the object-valued attribute backing the pool and winning over dataSourceJNDI, the actionable string rejection, and both dispatch routes. --- .../apache/tomcat/jdbc/pool/DataSourceFactory.java | 62 ++++- .../TestDataSourceFactoryRefAddrContent.java | 303 +++++++++++++++++++++ webapps/docs/changelog.xml | 13 + 3 files changed, 371 insertions(+), 7 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java index e601810c85..974c64dc59 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java @@ -389,16 +389,34 @@ public class DataSourceFactory implements ObjectFactory { Properties properties = new Properties(); + DataSource dataSourceObject = null; for (int i = 0; i < ALL_PROPERTIES.length; i++) { String propertyName = ALL_PROPERTIES[i]; RefAddr ra = ref.get(propertyName); if (ra != null) { - String propertyValue = ra.getContent().toString(); - properties.setProperty(propertyName, propertyValue); + Object content = ra.getContent(); + if (content == null) { + // RefAddr.getContent() is nullable, and a property with no + // content simply has no value to apply + continue; + } + if (PROP_DATASOURCE.equals(propertyName) && content instanceof DataSource) { + // A live data source cannot travel through Properties as a + // string, so keep the object and hand it to the pool below + dataSourceObject = (DataSource) content; + continue; + } + properties.setProperty(propertyName, content.toString()); } } - return createDataSource(properties, nameCtx, XA); + if (dataSourceObject == null) { + // no live data source involved - keep the legacy dispatch, so that + // subclasses overriding the three argument createDataSource() are + // still used for plain JNDI resources + return createDataSource(properties, nameCtx, XA); + } + return createDataSource(properties, nameCtx, XA, dataSourceObject); } /** @@ -687,10 +705,14 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_DATASOURCE); if (value != null) { - // This should never happen - throw new IllegalArgumentException( - "Can't set dataSource property as a string, this must be a javax.sql.DataSource object."); - + // Only genuinely string-valued dataSource attributes reach this + // point: getObjectInstance hands an actual javax.sql.DataSource + // RefAddr content straight to the pool instead of stringifying it. + // A string cannot be turned into a data source here. + throw new IllegalArgumentException("Can't set dataSource property as a string \"" + value + + "\". Either carry the javax.sql.DataSource object itself under the dataSource attribute" + + " of the Reference, or use the dataSourceJNDI attribute to name a data source bound in" + + " JNDI."); } value = properties.getProperty(PROP_DATASOURCE_JNDI); @@ -821,7 +843,33 @@ public class DataSourceFactory implements ObjectFactory { * @throws Exception if an error occurs creating the data source */ public DataSource createDataSource(Properties properties, Context context, boolean XA) throws Exception { + return createDataSource(properties, context, XA, null); + } + + /** + * Creates and configures a {@link DataSource} instance based on the given properties, with an optional live data + * source for the pool to wrap. + * <p> + * When {@code dataSourceObject} is not {@code null}, it is set on the pool configuration before the pool is + * created, and the {@code dataSourceJNDI} lookup is skipped: the pool is built around the object that was handed + * to the factory directly. + * </p> + * + * @param properties The datasource configuration properties + * @param context The JNDI context + * @param XA {@code true} to create an XA data source + * @param dataSourceObject A live {@link DataSource} the pool should wrap, or {@code null} + * + * @return The configured datasource + * + * @throws Exception if an error occurs creating the data source + */ + protected DataSource createDataSource(Properties properties, Context context, boolean XA, + DataSource dataSourceObject) throws Exception { PoolConfiguration poolProperties = parsePoolProperties(properties); + if (dataSourceObject != null) { + poolProperties.setDataSource(dataSourceObject); + } if (poolProperties.getDataSourceJNDI() != null && poolProperties.getDataSource() == null) { performJNDILookup(context, poolProperties); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryRefAddrContent.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryRefAddrContent.java new file mode 100644 index 0000000000..c02eac4890 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryRefAddrContent.java @@ -0,0 +1,303 @@ +/* + * 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.naming; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.ResultSet; +import java.util.Properties; + +import javax.naming.CompositeName; +import javax.naming.Context; +import javax.naming.RefAddr; +import javax.naming.Reference; +import javax.naming.StringRefAddr; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.DataSourceFactory; +import org.apache.tomcat.jdbc.pool.PoolProperties; + +/** + * {@code getObjectInstance} reads the property values out of the + * {@link RefAddr}s of the {@link Reference}. The {@code RefAddr} + * contract makes {@code getContent()} nullable and {@code Object}- + * valued, so the factory loop must survive a null content instead of + * stringifying blindly, and a {@code dataSource} attribute carrying an + * actual {@code javax.sql.DataSource} object must reach the pool as + * that object rather than as its {@code toString()}. + */ +class TestDataSourceFactoryRefAddrContent extends JdbcPoolBaseTest { + + private Reference poolReference() { + Reference ref = new Reference("javax.sql.DataSource", null, null); + ref.add(new StringRefAddr("jmxEnabled", "false")); + ref.add(new StringRefAddr("initialSize", "1")); + ref.add(new StringRefAddr("minIdle", "0")); + return ref; + } + + private DataSource managedPool(Object result) { + Assertions.assertInstanceOf(DataSource.class, result, + "the factory must return the pool"); + return manage((DataSource) result); + } + + private void assertPoolConnects(DataSource ds) throws Exception { + try (Connection c = ds.getConnection(); + ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next()); + } + } + + @Test + void nullContentRefAddrIsSkippedAndThePoolStillComesUp() throws Exception { + // RefAddr.getContent() is contractually nullable - a StringRefAddr + // can be created with null content. Pre-fix, the toString() call on + // the content aborted the whole factory call with a bare NPE. + Reference ref = poolReference(); + ref.add(new StringRefAddr("driverClassName", "org.h2.Driver")); + ref.add(new StringRefAddr("url", getUrl())); + ref.add(new StringRefAddr("username", "sa")); + ref.add(new StringRefAddr("password", "")); + ref.add(new StringRefAddr("maxActive", null)); + + DataSource ds = managedPool(new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/TestDS"), null, null)); + + // The empty attribute is skipped, not applied as anything: maxActive + // stays at its untouched default. + Assertions.assertEquals(PoolProperties.DEFAULT_MAX_ACTIVE, + ds.getPoolProperties().getMaxActive(), + "a RefAddr with null content must not set the property"); + // The other RefAddrs must still have been applied, and the pool must + // really work. + Assertions.assertEquals("sa", ds.getPoolProperties().getUsername()); + assertPoolConnects(ds); + Assertions.assertEquals(1, poolSessions(), + "the pool must establish the configured initial connection"); + } + + @Test + void objectValuedDataSourceRefAddrBacksThePool() throws Exception { + // A Reference can carry a live object under an attribute. Pre-fix, + // the content was stringified ("org.h2.jdbcx.JdbcDataSource@...") + // and then rejected as "not a javax.sql.DataSource object" although + // it was exactly that object. + Reference ref = poolReference(); + org.h2.jdbcx.JdbcDataSource backing = new org.h2.jdbcx.JdbcDataSource(); + backing.setUrl(getUrl()); + backing.setUser("sa"); + backing.setPassword(""); + ref.add(new ObjectContentRefAddr("dataSource", backing)); + + DataSource ds = managedPool(new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertSame(backing, ds.getPoolProperties().getDataSource(), + "the object carried by the dataSource RefAddr must back the pool"); + assertPoolConnects(ds); + Assertions.assertEquals(1, poolSessions(), + "the pool must establish connections through that data source"); + } + + @Test + void objectValuedDataSourceRefAddrTakesPrecedenceOverDataSourceJNDI() + throws Exception { + // Both ways of giving the pool a backing data source are present. + // The object handed through the Reference is the concrete one and + // must win; pre-fix this configuration died on the stringified + // dataSource attribute before any of that could be decided. + Reference ref = poolReference(); + org.h2.jdbcx.JdbcDataSource handed = new org.h2.jdbcx.JdbcDataSource(); + handed.setUrl(getUrl()); + handed.setUser("sa"); + handed.setPassword(""); + org.h2.jdbcx.JdbcDataSource bound = new org.h2.jdbcx.JdbcDataSource(); + bound.setUrl(getUrl()); + bound.setUser("sa"); + bound.setPassword(""); + ref.add(new ObjectContentRefAddr("dataSource", handed)); + ref.add(new StringRefAddr("dataSourceJNDI", "jdbc/BoundBacking")); + + DataSource ds = managedPool(new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/TestDS"), + contextReturning(bound), null)); + + Assertions.assertSame(handed, ds.getPoolProperties().getDataSource(), + "the object-valued dataSource attribute must win over dataSourceJNDI"); + assertPoolConnects(ds); + } + + @Test + void stringValuedDataSourceRefAddrIsRejectedWithAnActionableMessage() { + // A genuinely string-valued dataSource attribute still cannot become + // a data source, but the rejection must say what to do instead of + // contradicting the object-valued path. Pre-fix the message did not + // name the supported alternatives. + Reference ref = poolReference(); + ref.add(new StringRefAddr("dataSource", "com.example.NotADataSource")); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new DataSourceFactory().getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), null, null)); + Assertions.assertTrue(e.getMessage().contains("dataSourceJNDI"), + "the rejection must point at the dataSourceJNDI attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("com.example.NotADataSource"), + "the rejection must name the offending value, was: " + e.getMessage()); + } + + @Test + void nullContentDataSourceRefAddrIsSimplySkipped() throws Exception { + // The null-content skip must run before the object-valued special + // case, so a content-less attribute on the "dataSource" name is just + // an unset property - not a rejection. + Reference ref = poolReference(); + ref.add(new StringRefAddr("driverClassName", "org.h2.Driver")); + ref.add(new StringRefAddr("url", getUrl())); + ref.add(new StringRefAddr("username", "sa")); + ref.add(new StringRefAddr("password", "")); + ref.add(new StringRefAddr("dataSource", null)); + + DataSource ds = managedPool(new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertNull(ds.getPoolProperties().getDataSource(), + "a null-content dataSource attribute must leave the property unset"); + assertPoolConnects(ds); + } + + @Test + void plainReferenceStillDispatchesToTheThreeArgumentCreateDataSource() + throws Exception { + // Subclasses customising the public three argument createDataSource + // hook must stay in the loop for ordinary references; only a live + // object-valued dataSource attribute takes the four argument route. + class DispatchingFactory extends DataSourceFactory { + private boolean threeArgCalled; + + @Override + public javax.sql.DataSource createDataSource(Properties properties, + Context context, boolean xa) throws Exception { + threeArgCalled = true; + return super.createDataSource(properties, context, xa); + } + } + DispatchingFactory factory = new DispatchingFactory(); + + Reference ref = poolReference(); + ref.add(new StringRefAddr("driverClassName", "org.h2.Driver")); + ref.add(new StringRefAddr("url", getUrl())); + ref.add(new StringRefAddr("username", "sa")); + ref.add(new StringRefAddr("password", "")); + + DataSource ds = managedPool(factory.getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertTrue(factory.threeArgCalled, + "a plain reference must go through the three argument hook"); + assertPoolConnects(ds); + } + + @Test + void objectValuedReferenceUsesTheFourArgumentCreateDataSource() + throws Exception { + // The flip side: only a live data source object diverts from the + // legacy hook. + class DispatchingFactory extends DataSourceFactory { + private boolean threeArgCalled; + + @Override + public javax.sql.DataSource createDataSource(Properties properties, + Context context, boolean xa) throws Exception { + threeArgCalled = true; + return super.createDataSource(properties, context, xa); + } + } + DispatchingFactory factory = new DispatchingFactory(); + + Reference ref = poolReference(); + org.h2.jdbcx.JdbcDataSource backing = new org.h2.jdbcx.JdbcDataSource(); + backing.setUrl(getUrl()); + backing.setUser("sa"); + backing.setPassword(""); + ref.add(new ObjectContentRefAddr("dataSource", backing)); + + DataSource ds = managedPool(factory.getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertFalse(factory.threeArgCalled, + "an object-valued dataSource must use the four argument route"); + Assertions.assertSame(backing, ds.getPoolProperties().getDataSource()); + assertPoolConnects(ds); + } + + /* + * A RefAddr carrying an arbitrary live object as its content, as the + * RefAddr contract allows (getContent() returns Object). + */ + private static class ObjectContentRefAddr extends RefAddr { + private static final long serialVersionUID = 1L; + private final Object content; + + ObjectContentRefAddr(String type, Object content) { + super(type); + this.content = content; + } + + @Override + public Object getContent() { + return content; + } + } + + /* + * A JNDI context whose lookup returns the given object for any name, + * standing in for a bound data source. + */ + private static Context contextReturning(final Object bound) { + return (Context) Proxy.newProxyInstance( + TestDataSourceFactoryRefAddrContent.class.getClassLoader(), + new Class<?>[] { Context.class }, + new InvocationHandler() { + @Override + public Object invoke(Object proxy, java.lang.reflect.Method method, + Object[] args) { + switch (method.getName()) { + case "lookup": + return bound; + case "hashCode": + return System.identityHashCode(proxy); + case "equals": + return proxy == args[0]; + case "toString": + return "ContextStub"; + case "close": + return null; + default: + throw new UnsupportedOperationException(method.getName()); + } + } + }); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 5b13d54b06..2d310cb1cb 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1099,6 +1099,19 @@ releases it, as before, when it leaves the pool. (remm) </fix> + <fix> + Correct a defect that meant the JNDI factory for the JDBC pool aborted + with a <code>NullPointerException</code> when a resource reference + carried an attribute with null content, and that made it impossible to + give the pool a live <code>javax.sql.DataSource</code> through a + reference: an object-valued <code>dataSource</code> attribute was + stringified and then rejected with a message contradicting the + documented property type. Attributes with null content are now skipped + and an object-valued <code>dataSource</code> attribute is used to back + the pool, taking precedence over <code>dataSourceJNDI</code>; a string + value for that attribute is still rejected, with a message that now + points at the supported ways of providing a data source. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
