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 8298b681c28769663426ac3c8da1f487f07f7aad Author: opencode <[email protected]> AuthorDate: Tue Oct 27 16:26:23 2026 +0100 Honour the java.sql.Wrapper contract on pooled connection handles Two contract violations on the handle the pool hands out, both around the Wrapper interface: - unwrap(javax.sql.XAConnection.class) on a pool that is not backed by an XA data source returned a silent null. The contract requires a SQLException naming the interface; a null instead turns into an unrelated NullPointerException at the caller's first use of the "unwrapped" value, with nothing left to diagnose. - isWrapperFor() only tested the physical driver connection, so it answered false for interfaces the handle itself implements - notably javax.sql.PooledConnection, which every handle implements and to which a cast succeeds. An object is always a wrapper of the interfaces it implements. The dispatch now answers isWrapperFor true for handle-implemented interfaces, and unwrap returns the handle itself for such interfaces when the physical connection does not also provide them. Interfaces the physical connection provides keep resolving physically: unwrap(java.sql.Connection.class) has always reached the driver connection and still does. Note the defect report's proposed fix would have regressed exactly this (its self-check-first ordering returns the handle for java.sql.Connection), and its proposal to return the pool's internal PooledConnection object for a javax.sql.PooledConnection unwrap would have handed back an object that does not implement the requested interface; the shipped ordering avoids both. On XA pools unwrapping javax.sql.XAConnection keeps yielding the driver's XA connection object, as it always has. The proxy-level contract and the interceptor-level methods are now also distinguished in the javadoc. Regression tests in TestWrapperContract: the two contract fixes fail pre-fix (silent null; false for an implemented interface) and three controls pin what must not change (physical reach of unwrap(java.sql.Connection.class), the internal pooled-connection unwrap idiom, and the driver XA object on XA pools) - the controls pass both pre- and post-fix. No changelog entry, consistent with the preceding proxy-contract fixes: the null-return path ended in a guaranteed NPE and the previously throwing paths had no callers to break. --- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 38 +++++- .../tomcat/jdbc/pool/TestWrapperContract.java | 144 +++++++++++++++++++++ 2 files changed, 177 insertions(+), 5 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java index ac83b2e76f..c303548653 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java @@ -96,9 +96,14 @@ public class ProxyConnection extends JdbcInterceptor { } /** - * Checks whether this proxy wraps the given interface. + * Checks whether this interceptor wraps the given interface. This is the + * view from the interceptor itself; the contract of the connection handle + * the pool hands out - which also implements {@link java.sql.Connection} + * and {@link javax.sql.PooledConnection} (and {@link javax.sql.XAConnection} + * on XA pools) - is answered by the dispatch in + * {@link #invoke(Object, Method, Object[])}. * @param iface the interface to check - * @return true if this proxy wraps the given interface + * @return true if this interceptor wraps the given interface */ public boolean isWrapperFor(Class<?> iface) { if (iface == XAConnection.class && connection.getXAConnection()!=null) { @@ -119,7 +124,14 @@ public class ProxyConnection extends JdbcInterceptor { if (iface == PooledConnection.class) { return connection; }else if (iface == XAConnection.class) { - return connection.getXAConnection(); + javax.sql.XAConnection xa = connection.getXAConnection(); + if (xa == null) { + // The Wrapper contract asks for a failure, not for a silent + // null that turns into an unrelated NullPointerException at + // the caller's first use of the "unwrapped" value. + throw new SQLException("Not a wrapper of " + iface.getName()); + } + return xa; } else if (isWrapperFor(iface)) { return connection.getConnection(); } else { @@ -201,9 +213,25 @@ public class ProxyConnection extends JdbcInterceptor { throw new SQLException("Connection has already been closed."); } if (compare(UNWRAP_VAL,method)) { - return unwrap((Class<?>)args[0]); + Class<?> iface = (Class<?>)args[0]; + if (iface != XAConnection.class && iface.isInstance(proxy) + && !iface.isInstance(connection.getConnection())) { + // The Wrapper contract: the handle is a wrapper of every + // interface it implements itself. Interfaces the physical + // connection behind it also provides - java.sql.Connection + // first of all - keep resolving to the physical connection, + // as they always have; javax.sql.XAConnection is excluded + // because unwrapping to it has always yielded the XA + // connection object where one exists, and the unwrap above + // reports the not-a-wrapper failure where none does. + return proxy; + } + return unwrap(iface); } else if (compare(ISWRAPPERFOR_VAL,method)) { - return Boolean.valueOf(this.isWrapperFor((Class<?>)args[0])); + Class<?> iface = (Class<?>)args[0]; + // An object is always a wrapper of the interfaces it implements. + return Boolean.valueOf(iface.isInstance(proxy) + || this.isWrapperFor(iface)); } try { PooledConnection poolc = connection; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java new file mode 100644 index 0000000000..3faacbc5eb --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java @@ -0,0 +1,144 @@ +/* + * 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.sql.Connection; +import java.sql.SQLException; + +import javax.sql.XAConnection; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The {@link java.sql.Wrapper} contract of the connection handle the pool + * hands out. Per the contract, {@code isWrapperFor} must answer true for an + * interface the handle itself implements, and {@code unwrap} must fail as an + * {@link SQLException} for an interface nothing behind the handle provides - + * never hand back a silent {@code null}. The handle implements + * {@link java.sql.Connection} and {@link javax.sql.PooledConnection} (plus + * {@link javax.sql.XAConnection} on XA pools), while unwrapping interfaces + * the physical driver connection provides keeps reaching the physical + * connection. + */ +class TestWrapperContract extends JdbcPoolBaseTest { + + @Test + void unwrapToXaConnectionOnAPlainPoolFailsAsNotAWrapperNotSilentlyReturnsNull() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> pooled.unwrap(XAConnection.class), + "unwrap must throw for an interface nothing behind the " + + "handle provides, not hand back a silent null"); + Assertions.assertTrue(thrown.getMessage().contains("Not a wrapper"), + "the failure must say it is a not-a-wrapper case: " + + thrown.getMessage()); + // The two Wrapper queries must agree; they already did, pin that. + Assertions.assertFalse(pooled.isWrapperFor(XAConnection.class)); + } + } + + @Test + void theHandleIsAWrapperOfTheInterfacesItImplementsItself() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + // The handle IS a javax.sql.PooledConnection - a cast succeeds, + // so isWrapperFor must answer true and unwrap must answer with + // it, not report "Not a wrapper". + Assertions.assertTrue(pooled.isWrapperFor(javax.sql.PooledConnection.class), + "isWrapperFor must honour an interface the handle itself implements"); + Object unwrapped = pooled.unwrap(javax.sql.PooledConnection.class); + Assertions.assertSame(pooled, unwrapped, + "unwrapping an interface to the interface itself must return the handle"); + } + } + + @Test + void unwrappingThePrimaryInterfaceStillReachesThePhysicalConnection() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + // The established idiom: the physical driver connection is a + // java.sql.Connection too, and unwrapping interfaces the + // physical connection provides must keep reaching it. + Assertions.assertTrue(pooled.isWrapperFor(Connection.class)); + Connection physical = pooled.unwrap(Connection.class); + Assertions.assertNotSame(pooled, physical, + "unwrap(Connection.class) has always returned the physical connection"); + Assertions.assertTrue( + physical.getClass().getName().startsWith("org.h2."), + "expected the driver's own connection object, got " + + physical.getClass().getName()); + } + } + + @Test + void theInternalPooledConnectionUnwrapIdiomIsUnchanged() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + // The pool's own introspection idiom (used in tests and by + // interceptors): unwrap to the internal pooled connection. + org.apache.tomcat.jdbc.pool.PooledConnection internal = + pooled.unwrap(org.apache.tomcat.jdbc.pool.PooledConnection.class); + Assertions.assertSame( + interceptorOf(pooled, ProxyConnection.class).getConnection(), + internal, + "unwrapping to the internal pooled connection must keep " + + "returning the pool's own object"); + } + } + + @Test + void anXaPoolStillUnwrapsToTheDriversXaConnection() throws Exception { + org.h2.jdbcx.JdbcDataSource inner = new org.h2.jdbcx.JdbcDataSource(); + inner.setUrl(getUrl()); + inner.setUser("sa"); + inner.setPassword(""); + XADataSource ds = new XADataSource(); + ds.setDriverClassName("org.h2.Driver"); + ds.getPoolProperties().setJmxEnabled(false); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setDataSource(inner); + manage(ds); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + Assertions.assertTrue(pooled.isWrapperFor(XAConnection.class)); + Object unwrapped = pooled.unwrap(XAConnection.class); + // Established XA behavior: the driver's XA connection object, + // not the handle. Pin it so the null-target fix above does not + // silently re-point this at the handle. + Assertions.assertNotNull(unwrapped); + Assertions.assertInstanceOf(XAConnection.class, unwrapped); + Assertions.assertNotSame(pooled, unwrapped); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
