This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 008b6e2186d4cdac471dfcaa402ba028789ea572 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 00:38:09 2026 +0100 Name container-managed connection pools uniquely Singleton JNDI resources built by the Tomcat JDBC pool factory that do not configure the pool's object_name attribute are now given a container-generated name of the form domain/host/context-path/JNDI-name (webapp resources) or domain/JNDI-name (global resources), so container-managed pools no longer share a pool name. --- java/org/apache/catalina/core/Constants.java | 33 ++ .../apache/catalina/core/LocalStrings.properties | 2 + .../catalina/core/NamingContextListener.java | 152 ++++++++- .../catalina/core/TestNamingContextListener.java | 347 +++++++++++++++++++++ webapps/docs/changelog.xml | 9 + 5 files changed, 534 insertions(+), 9 deletions(-) diff --git a/java/org/apache/catalina/core/Constants.java b/java/org/apache/catalina/core/Constants.java index 1e74583dda..1b6234c781 100644 --- a/java/org/apache/catalina/core/Constants.java +++ b/java/org/apache/catalina/core/Constants.java @@ -16,6 +16,8 @@ */ package org.apache.catalina.core; +import java.util.Set; + /** * Constants used by the core Catalina package. */ @@ -41,4 +43,35 @@ public class Constants { * The fully qualified class name of the JSP servlet. */ public static final String JSP_SERVLET_CLASS = "org.apache.jasper.servlet.JspServlet"; + + /** + * The fully qualified class name of the JNDI resource factory of the + * Tomcat JDBC connection pool. Referenced by name only: catalina.jar has + * no compile-time dependency on tomcat-jdbc.jar. + */ + public static final String TOMCAT_JDBC_POOL_FACTORY = + "org.apache.tomcat.jdbc.pool.DataSourceFactory"; + + /** + * System property consulted to determine the default factory of a + * {@code javax.sql.DataSource} resource declared without a + * {@code factory} attribute. + */ + public static final String DATASOURCE_FACTORY_SYSPROP = "javax.sql.DataSource.Factory"; + + /** + * Name of the pool attribute that carries the pool's registration name. + * Mirrors the {@code OBJECT_NAME} constant of the pool's JNDI resource + * factory. + */ + public static final String POOL_NAME_PARAMETER = "object_name"; + + /** + * The resource type names the Tomcat JDBC pool's JNDI resource factory + * builds data sources for. + */ + public static final Set<String> POOL_DATASOURCE_TYPES = Set.of( + "javax.sql.DataSource", "javax.sql.XADataSource", + "org.apache.tomcat.jdbc.pool.DataSource", + "org.apache.tomcat.jdbc.pool.XADataSource"); } diff --git a/java/org/apache/catalina/core/LocalStrings.properties b/java/org/apache/catalina/core/LocalStrings.properties index 47111b3f7f..d1b2d3b8d5 100644 --- a/java/org/apache/catalina/core/LocalStrings.properties +++ b/java/org/apache/catalina/core/LocalStrings.properties @@ -167,6 +167,8 @@ naming.addEnvEntry=Adding environment entry [{0}] naming.addResourceEnvRef=Adding resource env ref [{0}] naming.addResourceLink=Adding resource link [{0}] naming.addResourceRef=Adding resource reference [{0}] at [{1}] +naming.addResourceRefNoPoolName=No connection pool name was generated for resource [{0}], the pool keeps its own generated name +naming.addResourceRefPoolName=Resource [{0}] in context [{1}] is named [{2}] for the connection pool naming.addService=Adding service reference [{0}] at [{1}] naming.addSlash=Changing service URL to [/{0}] naming.bind=Bind naming context to container [{0}] diff --git a/java/org/apache/catalina/core/NamingContextListener.java b/java/org/apache/catalina/core/NamingContextListener.java index ea94bcb2d7..fce91c0511 100644 --- a/java/org/apache/catalina/core/NamingContextListener.java +++ b/java/org/apache/catalina/core/NamingContextListener.java @@ -612,15 +612,7 @@ public class NamingContextListener implements LifecycleListener, PropertyChangeL */ protected ObjectName createObjectName(ContextResource resource) throws MalformedObjectNameException { - String domain = null; - if (container instanceof StandardServer) { - domain = ((StandardServer) container).getDomain(); - } else if (container instanceof ContainerBase) { - domain = ((ContainerBase) container).getDomain(); - } - if (domain == null) { - domain = "Catalina"; - } + String domain = domainOrDefault(); ObjectName name = null; String quotedResourceName = ObjectName.quote(resource.getName()); @@ -991,6 +983,8 @@ public class NamingContextListener implements LifecycleListener, PropertyChangeL StringRefAddr refAddr = new StringRefAddr(paramName, paramValue); ref.add(refAddr); } + // Give a pool that does not name itself a container-unique name. + addContainerPoolName(ref, resource); } try { @@ -1023,6 +1017,146 @@ public class NamingContextListener implements LifecycleListener, PropertyChangeL } + /** + * Pass a container-managed JNDI resource that the Tomcat JDBC connection + * pool will build, and that does not name itself, a container-unique pool + * registration name through the pool factory's {@code object_name} + * attribute. + * + * @param ref the reference created for the resource + * @param resource the resource descriptor + */ + protected void addContainerPoolName(Reference ref, ContextResource resource) { + // Never disturb a resource that names itself. + if (resource.getProperty(Constants.POOL_NAME_PARAMETER) != null) { + return; + } + // The pool factory builds only these resource classes. + if (!Constants.POOL_DATASOURCE_TYPES.contains(resource.getType())) { + return; + } + // Per-lookup pools already get a distinct generated name each; sharing + // one injected name across the instances would merge distinct live + // pools, the exact situation a container-unique name is meant to end. + if (!resource.getSingleton()) { + return; + } + if (!isTomcatJdbcPoolFactory(resource)) { + return; + } + String poolName = generatePoolName(resource); + if (poolName == null) { + if (log.isDebugEnabled()) { + log.debug(sm.getString("naming.addResourceRefNoPoolName", resource.getName())); + } + return; + } + ref.add(new StringRefAddr(Constants.POOL_NAME_PARAMETER, poolName)); + if (log.isDebugEnabled()) { + log.debug(sm.getString("naming.addResourceRefPoolName", resource.getName(), getName(), poolName)); + } + } + + + /** + * Does the factory that will build this resource's data source resolve to + * the Tomcat JDBC pool factory? + * <p> + * The match is on the exact class name, so a factory that subclasses the + * pool factory is not matched. Such a factory keeps its current behaviour + * and remains responsible for naming its pools itself. + * + * @param resource the resource descriptor + * + * @return <code>true</code> if the resource is built by the Tomcat JDBC + * pool factory + */ + private boolean isTomcatJdbcPoolFactory(ContextResource resource) { + // The factory attribute of a Resource element is stored as a parameter + // and reaches the Reference as its factory. + Object factory = resource.getProperty("factory"); + if (factory == null) { + // With no factory attribute the container default + // org.apache.naming.factory.ResourceFactory answers for + // javax.sql.DataSource only, and delegates to the system property, + // DBCP2 by default. Mirror that resolution exactly. Other resource + // types have no delegating default and are not pool-built. + if ("javax.sql.DataSource".equals(resource.getType())) { + factory = System.getProperty(Constants.DATASOURCE_FACTORY_SYSPROP, + org.apache.naming.factory.Constants.DBCP_DATASOURCE_FACTORY); + } + } + return Constants.TOMCAT_JDBC_POOL_FACTORY.equals(factory); + } + + + /** + * Generate the unique connection pool name for a resource of this + * container. + * <p> + * Webapp resources are named {@code <domain>/<host>/<context>/<jndiName>} + * and global resources {@code <domain>/<jndiName>}, where the JNDI name of + * a global resource is already unique within the Server's global + * namespace. Only the slash separator is used, a character with no + * meaning in a JMX object name or in a naming path; the pool's JNDI + * factory stores the name through {@link ObjectName#quote(String)} + * exactly as it stores any configured {@code object_name}. The + * generation is deterministic: the same configuration always yields the + * same name, across rebinds and context reloads. + * + * @param resource the resource descriptor + * + * @return the generated name, or <code>null</code> if this container's + * position does not allow a stable unique name to be generated + */ + protected String generatePoolName(ContextResource resource) { + if (container instanceof Context ctx) { + Container host = ctx.getParent(); + if (!(host instanceof Host)) { + // Without a Host above it, this context's position is not + // predictable: leave the pool at its own generated name. + return null; + } + // The context name already carries the "##<version>" suffix of a + // parallel deployment, so each versioned instance of a path gets + // its own distinct pool name. + String path = ctx.getName(); + if (path.startsWith("/")) { + path = path.substring(1); + } + return domainOrDefault() + "/" + host.getName() + "/" + path + "/" + resource.getName(); + } + if (container instanceof Server) { + // A global resource is shared by every webapp that links to it. It + // must be named by the one container that owns it, and its JNDI + // name is unique within that Server's global namespace. + return domainOrDefault() + "/" + resource.getName(); + } + return null; + } + + + /** + * The MBean domain of the associated container, defaulting to + * {@code Catalina} the way {@link #createObjectName(ContextResource)} + * does, which an embedded container without an Engine above it also gets. + * + * @return the domain, never <code>null</code> + */ + private String domainOrDefault() { + String domain = null; + if (container instanceof StandardServer) { + domain = ((StandardServer) container).getDomain(); + } else if (container instanceof ContainerBase) { + domain = ((ContainerBase) container).getDomain(); + } + if (domain == null) { + domain = "Catalina"; + } + return domain; + } + + /** * Set the specified resources in the naming context. * diff --git a/test/org/apache/catalina/core/TestNamingContextListener.java b/test/org/apache/catalina/core/TestNamingContextListener.java index 73b4d52b02..dfabc3193e 100644 --- a/test/org/apache/catalina/core/TestNamingContextListener.java +++ b/test/org/apache/catalina/core/TestNamingContextListener.java @@ -17,7 +17,10 @@ package org.apache.catalina.core; import javax.naming.InitialContext; +import javax.naming.NameNotFoundException; import javax.naming.NamingException; +import javax.naming.RefAddr; +import javax.naming.Reference; import jakarta.servlet.ServletContextEvent; import jakarta.servlet.ServletContextListener; @@ -26,10 +29,13 @@ import org.junit.Assert; import org.junit.Test; import org.apache.catalina.Context; +import org.apache.catalina.Host; import org.apache.catalina.LifecycleState; +import org.apache.catalina.deploy.NamingResourcesImpl; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; import org.apache.tomcat.util.descriptor.web.ContextEnvironment; +import org.apache.tomcat.util.descriptor.web.ContextResource; public class TestNamingContextListener extends TomcatBaseTest { @@ -166,4 +172,345 @@ public class TestNamingContextListener extends TomcatBaseTest { } } } + + + /* + * Tests for the container-generated connection pool name. The naming + * listener passes a container-unique name through the pool factory's + * object_name attribute to a JNDI resource that the Tomcat JDBC + * connection pool builds and that does not name itself. + * + * The pool classes are not on the main test suite's classpath, so the + * injection hook is exercised directly - it takes and returns only + * naming and descriptor types - which also keeps these tests independent + * of whether a pool could be instantiated here at all. + */ + + private static ContextResource poolResource(String jndiName, String factory) { + ContextResource resource = new ContextResource(); + resource.setName(jndiName); + resource.setType("javax.sql.DataSource"); + resource.setAuth("Container"); + resource.setScope("Shareable"); + if (factory != null) { + // The factory attribute of a Resource element is stored as a + // parameter, the way the digester rule stores it. + resource.setProperty("factory", factory); + } + return resource; + } + + /* + * The object_name RefAddr the listener injects for the resource when its + * reference is built, or null when it leaves the reference alone. + */ + private static RefAddr injectedPoolName(Object container, ContextResource resource) { + NamingContextListener listener = new NamingContextListener(); + listener.container = container; + Reference ref = new Reference(resource.getType()); + listener.addContainerPoolName(ref, resource); + return ref.get(Constants.POOL_NAME_PARAMETER); + } + + @Test + public void testWebappPoolResourceGetsContainerUniqueName() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + RefAddr addr = injectedPoolName(ctx, resource); + + Assert.assertNotNull("a pool that does not name itself must be named by the container", addr); + Assert.assertEquals( + tomcat.getEngine().getName() + "/" + tomcat.getHost().getName() + "/app1/jdbc/DS", + addr.getContent()); + } + + @Test + public void testRootContextKeepsEmptySegment() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = getProgrammaticRootContext(); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + RefAddr addr = injectedPoolName(ctx, resource); + + Assert.assertNotNull(addr); + // The segment structure never collapses: ROOT contributes an empty + // but kept segment, so its form cannot alias another context's. + Assert.assertEquals( + tomcat.getEngine().getName() + "/" + tomcat.getHost().getName() + "//jdbc/DS", + addr.getContent()); + } + + @Test + public void testParallelDeploymentSeparatesNames() throws Exception { + Tomcat tomcat = getTomcatInstance(); + // In this container the version marker of a parallel deployment is + // part of the context name, so the generated pool name of the two + // instances of one path must differ the same way. + Context instance1 = tomcat.addContext("/app1", null); + Context instance2 = tomcat.addContext("/app1##v2", null); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + String name1 = (String) injectedPoolName(instance1, resource).getContent(); + String name2 = (String) injectedPoolName(instance2, resource).getContent(); + + Assert.assertNotEquals(name1, name2); + Assert.assertEquals( + tomcat.getEngine().getName() + "/" + tomcat.getHost().getName() + "/app1/jdbc/DS", + name1); + Assert.assertEquals( + tomcat.getEngine().getName() + "/" + tomcat.getHost().getName() + "/app1##v2/jdbc/DS", + name2); + } + + @Test + public void testGlobalResourceGetsDomainScopedName() throws Exception { + Tomcat tomcat = getTomcatInstance(); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + RefAddr addr = injectedPoolName(tomcat.getServer(), resource); + + Assert.assertNotNull(addr); + // A global resource is named by the Server that owns it. Its JNDI + // name is already unique within the Server's global namespace. + Assert.assertEquals(tomcat.getEngine().getName() + "/jdbc/DS", addr.getContent()); + } + + @Test + public void testTwoWebappsWithSameJndiNameGetDifferentNames() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx1 = tomcat.addContext("/app1", null); + Context ctx2 = tomcat.addContext("/app2", null); + + Host otherHost = new StandardHost(); + otherHost.setName("example"); + tomcat.getEngine().addChild(otherHost); + StandardContext ctx3 = new StandardContext(); + ctx3.setPath("/app1"); + otherHost.addChild(ctx3); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + String name1 = (String) injectedPoolName(ctx1, resource).getContent(); + String name2 = (String) injectedPoolName(ctx2, resource).getContent(); + String name3 = (String) injectedPoolName(ctx3, resource).getContent(); + + // The case the JNDI name alone cannot serve: the same JNDI name under + // different paths or hosts must never produce the same pool name. + Assert.assertNotEquals(name1, name2); + Assert.assertNotEquals(name1, name3); + Assert.assertNotEquals(name2, name3); + Assert.assertEquals(tomcat.getEngine().getName() + "/localhost/app1/jdbc/DS", name1); + Assert.assertEquals(tomcat.getEngine().getName() + "/localhost/app2/jdbc/DS", name2); + Assert.assertEquals(tomcat.getEngine().getName() + "/example/app1/jdbc/DS", name3); + } + + @Test + public void testConfiguredObjectNameIsNotOverridden() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + resource.setProperty(Constants.POOL_NAME_PARAMETER, "My Pool"); + + Assert.assertNull("a resource that names itself must keep its own name", + injectedPoolName(ctx, resource)); + } + + @Test + public void testDefaultDbcpFactoryIsSkipped() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + String saved = System.getProperty(Constants.DATASOURCE_FACTORY_SYSPROP); + System.clearProperty(Constants.DATASOURCE_FACTORY_SYSPROP); + try { + ContextResource resource = poolResource("jdbc/DS", null); + Assert.assertNull("the DBCP default factory must not be renamed", + injectedPoolName(ctx, resource)); + } finally { + if (saved != null) { + System.setProperty(Constants.DATASOURCE_FACTORY_SYSPROP, saved); + } + } + } + + @Test + public void testSysPropResolvedPoolFactoryIsNamed() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + String saved = System.getProperty(Constants.DATASOURCE_FACTORY_SYSPROP); + System.setProperty(Constants.DATASOURCE_FACTORY_SYSPROP, Constants.TOMCAT_JDBC_POOL_FACTORY); + try { + ContextResource resource = poolResource("jdbc/DS", null); + RefAddr addr = injectedPoolName(ctx, resource); + Assert.assertNotNull( + "a resource whose factory resolves to the pool factory through the system" + + " property must be named like an explicitly configured pool", + addr); + Assert.assertEquals( + tomcat.getEngine().getName() + "/" + tomcat.getHost().getName() + "/app1/jdbc/DS", + addr.getContent()); + } finally { + if (saved == null) { + System.clearProperty(Constants.DATASOURCE_FACTORY_SYSPROP); + } else { + System.setProperty(Constants.DATASOURCE_FACTORY_SYSPROP, saved); + } + } + } + + @Test + public void testNonSingletonIsSkipped() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + resource.setSingleton(false); + + Assert.assertNull("per-lookup pools must keep their own distinct names", + injectedPoolName(ctx, resource)); + } + + @Test + public void testOtherFactoryIsSkipped() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + ContextResource resource = poolResource("jdbc/DS", "org.apache.naming.factory.BeanFactory"); + + Assert.assertNull("a resource built by another factory must not carry a pool attribute", + injectedPoolName(ctx, resource)); + } + + @Test + public void testNonDataSourceTypeIsSkipped() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = tomcat.addContext("/app1", null); + + ContextResource resource = poolResource("beans/Integer", Constants.TOMCAT_JDBC_POOL_FACTORY); + resource.setType("java.lang.Integer"); + + Assert.assertNull("the pool factory builds only the data source types", + injectedPoolName(ctx, resource)); + } + + @Test + public void testContextWithoutHostParentIsSkipped() throws Exception { + StandardContext orphan = new StandardContext(); + orphan.setName("/app1"); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + Assert.assertNull("an unpredictable container position must leave the pool at its own name", + injectedPoolName(orphan, resource)); + } + + @Test + public void testContextWithoutEngineUsesDefaultDomain() throws Exception { + StandardHost host = new StandardHost(); + host.setName("embedded-host"); + StandardContext ctx = new StandardContext(); + ctx.setPath("/ctx"); + host.addChild(ctx); + + ContextResource resource = poolResource("jdbc/DS", Constants.TOMCAT_JDBC_POOL_FACTORY); + + RefAddr addr = injectedPoolName(ctx, resource); + + Assert.assertNotNull(addr); + // Same default the container's own ObjectNames use when no Engine + // answers above the context. + Assert.assertEquals("Catalina/embedded-host/ctx/jdbc/DS", addr.getContent()); + } + + /* + * A LookupRef-backed resource (the lookupName attribute) must pass + * through the injection hook untouched: the bound reference is the lookup + * reference and the lookup resolves to the target, exactly as before pool + * naming existed. The reference it produces is not even a ResourceRef, so + * no factory attribute and no pool name is involved. + */ + @Test + public void testLookupRefResourceIsUntouched() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = getProgrammaticRootContext(); + + tomcat.enableNaming(); + + ContextEnvironment target = new ContextEnvironment(); + target.setType(String.class.getName()); + target.setName("env/target"); + target.setValue("lookup-target"); + ctx.getNamingResources().addEnvironment(target); + + ContextResource resource = poolResource("res/looked-up", Constants.TOMCAT_JDBC_POOL_FACTORY); + resource.setType(String.class.getName()); + resource.setLookupName("java:comp/env/env/target"); + ctx.getNamingResources().addResource(resource); + + ctx.addApplicationListener(LookupRefListener.class.getName()); + + tomcat.start(); + + Assert.assertEquals(LifecycleState.STARTED, ctx.getState()); + } + + public static final class LookupRefListener implements ServletContextListener { + + @Override + public void contextInitialized(ServletContextEvent sce) { + try { + javax.naming.Context envCtx = + (javax.naming.Context) new InitialContext().lookup("java:comp/env"); + if (!"lookup-target".equals(envCtx.lookup("res/looked-up"))) { + throw new RuntimeException("LookupRef resource did not resolve to its target"); + } + } catch (NamingException e) { + throw new RuntimeException(e); + } + } + } + + /* + * A resource added to a started context - the runtime path through the + * naming resources' property-change events - goes through the injection + * hook just like the start-time path, and the reference is bound. + */ + @Test + public void testRuntimeAddedPoolResourceIsBound() throws Exception { + Tomcat tomcat = getTomcatInstance(); + Context ctx = getProgrammaticRootContext(); + + tomcat.enableNaming(); + + tomcat.start(); + + Assert.assertEquals(LifecycleState.STARTED, ctx.getState()); + + NamingResourcesImpl namingResources = ctx.getNamingResources(); + namingResources.addResource(poolResource("jdbc/runtime", Constants.TOMCAT_JDBC_POOL_FACTORY)); + + javax.naming.Context envCtx = + ((StandardContext) ctx).getNamingContextListener().getEnvContext(); + Assert.assertNotNull(envCtx); + + // The pool factory is not on the main test suite's classpath, so + // resolving the reference cannot produce the data source. What the + // test pins is the difference between "a reference is bound" and + // "nothing is bound". + try { + envCtx.lookup("jdbc/runtime"); + Assert.fail("Resolving the reference must not succeed without the pool classes"); + } catch (NameNotFoundException e) { + Assert.fail("The runtime-added resource was not bound"); + } catch (NamingException e) { + // Expected: bound, but the reference could not be resolved. + } + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index cb02902d54..4971cddfcd 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -491,6 +491,15 @@ types of resources, not just class files, are now handled in multi-release JAR files. (markt) </fix> + <fix> + Singleton JNDI resources built by the Tomcat JDBC pool factory that do + not configure the pool's <code>object_name</code> attribute are now + given a container-generated pool name, of the form + <code>domain/host/context-path/JNDI-name</code> for webapp resources + and <code>domain/JNDI-name</code> for global resources. Previously, + the pool generated a name that repeated across pools, which caused + pools to share the same name. (remm) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
