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 f84427284baabb995d37cec71e1c32776e4e8035 Author: opencode <[email protected]> AuthorDate: Mon Nov 2 19:33:15 2026 +0100 Harden the JNDI factory's attribute parsing and data source routing The JNDI object factory used to read a resource Reference leniently and report poorly: unparsed attribute values were silently dropped or misread as defaults, a cleared attribute could configure a property to its empty value instead of leaving it off, and the two routes by which a data source can back the pool - the object-valued dataSource attribute and the dataSourceJNDI lookup - each rejected or misused valid and invalid configurations with messages that named neither the attribute nor the offending value. A full review campaign (original defect report plus five clean-room review rounds) produced the changes now folded into this one, grouped here by subject. Parsing is now strict and self-describing. An attribute whose value is blank means "not configured" uniformly - booleans, numerics, the transaction isolation level, the catalog, driverClassName, jdbcInterceptors, validationQuery, object_name, dataSource and dataSourceJNDI all leave their property at the PoolProperties default when the value carries nothing but whitespace. A value the property cannot take fails the resource naming the attribute and quoting the value, instead of being silently ignored: unparseable booleans and numbers, numbers outside an int's range, unknown isolation levels (whose java.sql.Connection numeric form is now accepted alongside the names). Numbers, keywords, class names and JNDI names are applied trimmed; identifiers - catalog, validation query, pool name - are stored verbatim, since whitespace can be meaningful in them. The backing data source is validated on both routes, with context. The dataSource attribute now carries a live javax.sql.DataSource, XADataSource or ConnectionPoolDataSource object through to the pool instead of stringifying it into a rejection; a genuinely text-valued attribute fails with guidance pointing at the two routes that do work. A pool cannot wrap another pool: that case is diagnosed where the factory accepts the object, naming the attribute and the class, and on the lookup route as the declared NamingException rather than an unchecked failure from deep inside pool start. The object a dataSourceJNDI name resolves to is checked to be a data source the connect path can use, blank names mean unset, and the pool's own XA class name is accepted at the reference type gate. Around the factory: the interceptor list parser treats a redundant separator as cosmetic in every position - leading and doubled ones used to abort pool start with a message-less SQLException, while the same separator trailing was dropped - and PoolProperties keeps its credential fields authoritative across a connectionProperties reconfigure. Diagnostics were localized, leveled, and deduplicated; the unknown-attribute sweep can no longer desynchronize from an ALL_PROPERTIES a subclass extends; the PROP_USE_CON_LOCK constant was named for its property. The documentation was made true: the class javadoc no longer promises a DBCP BasicDataSource and now states the blank/strict/trim policy and the method contracts, and webapps/docs/jdbc-pool.xml no longer points operators at a dataSource-as-text route the factory rejects, documents the isolation attribute's numeric form, adds the previously undocumented useLock and object_name attributes, and distinguishes object_name from the SlowQueryReportJmx objectName attribute. jdbc-pool suite: 734 green. --- modules/jdbc-pool/doc/jdbc-pool.xml | 59 ++- .../apache/tomcat/jdbc/pool/DataSourceFactory.java | 454 ++++++++++++++++----- .../apache/tomcat/jdbc/pool/PoolProperties.java | 57 ++- .../tomcat/jdbc/pool/util/LocalStrings.properties | 11 +- .../naming/TestDataSourceFactoryJndiLookup.java | 97 +++++ .../TestDataSourceFactoryNoOpPropertyWarnings.java | 17 +- .../TestDataSourceFactoryRefAddrContent.java | 119 +++++- ...stDataSourceFactoryUnknownPropertyWarnings.java | 2 +- .../tomcat/jdbc/naming/TestNamingFactory.java | 311 ++++++++++++++ .../pool/TestJdbcInterceptorConfigParsing.java | 56 ++- .../tomcat/jdbc/pool/TestPoolProperties.java | 59 +++ 11 files changed, 1095 insertions(+), 147 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index f67fe4b734..aa998d70dd 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -190,6 +190,15 @@ <section name="Attributes"> <p>To provide a very simple switch to and from commons-dbcp and tomcat-jdbc-pool, Most attributes are the same and have the same meaning.</p> + <p>When the pool is configured through the JNDI resource factory, an attribute whose + value is blank leaves its property at the default: clearing an attribute in a + resource definition switches the property off rather than configuring it to the + empty value. A value the property cannot take - an unparseable number, an unknown + boolean or transaction isolation level - fails the creation of the pool with an + error naming the attribute, rather than being silently ignored. Numeric, boolean + and keyword values, class names and JNDI names are read ignoring surrounding + whitespace; names, queries and URLs are stored verbatim, since a space can be + part of such a value.</p> <subsection name="JNDI Factory and Type"> <attributes> <attribute name="factory" required="true"> @@ -199,8 +208,12 @@ <p>Type should always be <code>javax.sql.DataSource</code> or <code>javax.sql.XADataSource</code></p> <p>Depending on the type a <code>org.apache.tomcat.jdbc.pool.DataSource</code> or a <code>org.apache.tomcat.jdbc.pool.XADataSource</code> will be created.</p> <p>Note that the <code>javax.sql.XADataSource</code> type only provides XA connections if the pool is backed - by an XA data source: the <code>dataSource</code> or <code>dataSourceJNDI</code> attribute must name a - <code>javax.sql.XADataSource</code> provided by the JDBC driver. A pool configured with + by an XA data source: the data source the pool takes its connections from - supplied through the + <code>dataSourceJNDI</code> or <code>dataSource</code> attribute - must implement + <code>javax.sql.XADataSource</code>. In an XML resource definition an attribute can only carry text, + while the <code>dataSource</code> attribute takes the data source object itself, so the XML route to a + backing data source is <code>dataSourceJNDI</code>, naming a <code>javax.sql.XADataSource</code> + provided by the JDBC driver and bound in JNDI. A pool configured with <code>driverClassName</code> and <code>url</code> pools plain JDBC connections, and a request for an XA connection from such a pool fails with an explicit error.</p> </attribute> @@ -247,7 +260,11 @@ <li><code>REPEATABLE_READ</code></li> <li><code>SERIALIZABLE</code></li> </ul> - <p>If not set, the method will not be called and it defaults to the JDBC driver.</p> + <p>The numeric transaction isolation level of <code>java.sql.Connection</code>, for example + <code>2</code> for <code>READ_COMMITTED</code>, is accepted in place of the name. The name or number + is matched ignoring surrounding whitespace, and any value other than the names above, their numbers + or a blank value fails the creation of the pool with an error naming the attribute.</p> + <p>If not set, or set to a blank value, the method will not be called and it defaults to the JDBC driver.</p> </attribute> <attribute name="defaultCatalog" required="false"> @@ -344,7 +361,9 @@ <p>(String) The SQL query that will be used to validate connections from this pool before returning them to the caller. If specified, this query does not have to return any data, it just can't throw an <code>SQLException</code>. The default value is <code>null</code>. - If not specified, connections will be validation by the isValid() method. + If not specified, or set to a blank value, connections will be validated by the + <code>isValid()</code> method of <code>java.sql.Connection</code>: a blank query is not sent to the + database as an empty statement. Example values are <code>SELECT 1</code>(mysql), <code>select 1 from dual</code>(oracle), <code>SELECT 1</code>(MS Sql Server) </p> </attribute> @@ -475,6 +494,16 @@ The default value is <code>true</code>.</p> </attribute> + <attribute name="object_name" required="false"> + <p>(String) The pool's name, used as the <code>name</code> key property of the JMX registrations of the + pool's MBean and of interceptors that register with the pool, such as + <code>SlowQueryReportJmx</code>. When the attribute is set through the JNDI resource factory, + surrounding whitespace is not stripped - the name is stored as it is written - and a blank value + leaves the pool with its generated name. Note that this attribute of the pool is spelled with an + underscore, unlike the <code>objectName</code> attribute of the <code>SlowQueryReportJmx</code> + interceptor described below, which overrides only that interceptor's registration.</p> + </attribute> + <attribute name="fairQueue" required="false"> <p>(boolean) Set to true if you wish that calls to getConnection should be treated fairly in a true FIFO fashion. This uses the <code>org.apache.tomcat.jdbc.pool.FairBlockingQueue</code> @@ -524,6 +553,15 @@ The default value is <code>true</code>. </p> </attribute> + <attribute name="useLock" required="false"> + <p>(boolean) Set to true if a lock should be used when operations are performed on the connection object. + Should be set to <code>false</code> unless you plan to have a background thread of your own doing idle + and abandon checking, such as a JMX client. If the pool sweeper is enabled - the pool's own background + maintenance thread, started by <code>timeBetweenEvictionRunsMillis</code> together with one of the + checks it runs - the lock is used regardless of this setting. + The default value is <code>false</code>. + </p> + </attribute> <attribute name="suspectTimeout" required="false"> <p>(int) Timeout value in seconds. Default value is <code>0</code>.<br/> Similar to to the <code>removeAbandonedTimeout</code> value but instead of treating the connection @@ -568,9 +606,16 @@ <p>(javax.sql.DataSource) Inject a data source to the connection pool, and the pool will use the data source to retrieve connections instead of establishing them using the <code>java.sql.Driver</code> interface. This is useful when you wish to pool XA connections or connections established using a data source instead of a connection string. Default value is <code>null</code> </p> + <p>When the pool is configured through the JNDI resource factory, this attribute carries the + data source object itself, and the object must be a <code>javax.sql.DataSource</code>, a + <code>javax.sql.XADataSource</code> or a <code>javax.sql.ConnectionPoolDataSource</code> provided by + the JDBC driver - and not another connection pool, which the pool refuses to wrap. A resource + definition can only give attributes as text, and text cannot carry a live object; a text value here + fails the creation of the pool. In a resource definition, use this attribute's JNDI counterpart, + <code>dataSourceJNDI</code>, to back the pool with a data source.</p> </attribute> <attribute name="dataSourceJNDI" required="false"> - <p>(String) The JNDI name for a data source to be looked up in JNDI and then used to establish connections to the database. See the <code>dataSource</code> attribute. Default value is <code>null</code>. If the name cannot be resolved when the pool is created, the creation fails with an error naming the unresolved name rather than producing a pool that cannot provide connections. + <p>(String) The JNDI name for a data source to be looked up in JNDI and then used to establish connections to the database. See the <code>dataSource</code> attribute. Default value is <code>null</code>. Surrounding whitespace around the name is ignored. If the name cannot be resolved when the pool is created, or resolves to an object the pool cannot establish connections from, the creation fails with an error naming the unresolved name rather than producing a pool that cannot provi [...] </p> </attribute> <attribute name="useDisposableConnectionFacade" required="false"> @@ -835,6 +880,10 @@ <p>(String) Define a valid <code>javax.management.ObjectName</code> string that will be used to register this object with the platform mbean server The default value is <code>null</code> and the object will be registered using tomcat.jdbc:type=org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx,name=the-name-of-the-pool + Note this interceptor attribute is spelled <code>objectName</code>, unlike the pool's own + <code>object_name</code> attribute, which names the pool in the registrations; a + <code>objectName</code> attribute on the pool itself is not a pool property and will be reported + as ignored. </p> </attribute> </attributes> 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 767a9e34ad..e3b1d5d3bb 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 @@ -42,22 +42,20 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** * <p> - * JNDI object factory that creates an instance of <code>BasicDataSource</code> that has been configured based on the - * <code>RefAddr</code> values of the specified <code>Reference</code>, which must match the names and data types of the - * <code>BasicDataSource</code> bean properties. + * JNDI object factory that creates an instance of + * {@link org.apache.tomcat.jdbc.pool.DataSource} - or {@link XADataSource}, when the + * reference names a type to be built as XA - configured from the {@code RefAddr} + * values of the specified {@code Reference}. Address names are matched against the + * pool properties listed in {@link #ALL_PROPERTIES}; they are the attributes of the + * pool documented with {@link PoolConfiguration}'s setters, not the property set of + * any other pool implementation. + * </p> + * <p> + * The factory applies a strict reading of the reference: an attribute whose value is + * blank leaves its property at the {@link PoolProperties} default, and a value the + * property cannot take fails the lookup with an exception naming the attribute + * instead of being silently ignored. * </p> - * <br> - * Properties available for configuration:<br> - * <a href="https://commons.apache.org/dbcp/configuration.html";>Commons DBCP properties</a><br> - * <ol> - * <li>initSQL - A query that gets executed once, right after the connection is established.</li> - * <li>testOnConnect - run validationQuery after connection has been established.</li> - * <li>validationInterval - avoid excess validation, only run validation at most at this frequency - time in - * milliseconds.</li> - * <li>jdbcInterceptors - a semicolon separated list of classnames extending {@link JdbcInterceptor} class.</li> - * <li>jmxEnabled - true of false, whether to register the pool with JMX.</li> - * <li>fairQueue - true of false, whether the pool should sacrifice a little bit of performance for true fairness.</li> - * </ol> */ public class DataSourceFactory implements ObjectFactory { /** @@ -261,7 +259,7 @@ public class DataSourceFactory implements ObjectFactory { /** * Property name for useLock. */ - protected static final String PROP_USE_CON_LOCK = "useLock"; + protected static final String PROP_USE_LOCK = "useLock"; /** * Property name for dataSource. @@ -343,7 +341,7 @@ public class DataSourceFactory implements ObjectFactory { PROP_ACCESSTOUNDERLYINGCONNECTIONALLOWED, PROP_REMOVEABANDONED, PROP_REMOVEABANDONEDTIMEOUT, PROP_LOGABANDONED, PROP_POOLPREPAREDSTATEMENTS, PROP_MAXOPENPREPAREDSTATEMENTS, PROP_CONNECTIONPROPERTIES, PROP_INITSQL, PROP_INTERCEPTORS, PROP_JMX_ENABLED, PROP_FAIR_QUEUE, PROP_USE_EQUALS, OBJECT_NAME, - PROP_ABANDONWHENPERCENTAGEFULL, PROP_MAXAGE, PROP_USE_CON_LOCK, PROP_DATASOURCE, PROP_DATASOURCE_JNDI, + PROP_ABANDONWHENPERCENTAGEFULL, PROP_MAXAGE, PROP_USE_LOCK, PROP_DATASOURCE, PROP_DATASOURCE_JNDI, PROP_SUSPECT_TIMEOUT, PROP_ALTERNATE_USERNAME_ALLOWED, PROP_COMMITONRETURN, PROP_ROLLBACKONRETURN, PROP_USEDISPOSABLECONNECTIONFACADE, PROP_LOGVALIDATIONERRORS, PROP_PROPAGATEINTERRUPTSTATE, PROP_IGNOREEXCEPTIONONPRELOAD, PROP_USESTATEMENTFACADE }; @@ -373,20 +371,14 @@ public class DataSourceFactory implements ObjectFactory { private static final Set<String> RESERVED_JNDI_ADDRESS_NAMES = Collections.unmodifiableSet( new HashSet<>(Arrays.asList("factory", "singleton", "scope", "auth", "description", "forceString"))); - /* - * The recognized property names as a set, for the sweep over the - * reference's addresses that reports everything the lookup loop above - * never read. - */ - private static final Set<String> ALL_PROPERTY_NAMES = Collections.unmodifiableSet( - new HashSet<>(Arrays.asList(ALL_PROPERTIES))); - // -------------------------------------------------- ObjectFactory Methods /** * <p> - * Create and return a new <code>BasicDataSource</code> instance. If no instance can be created, return - * <code>null</code> instead. + * Creates and returns the pooled data source that the given {@code Reference} + * describes: an {@link org.apache.tomcat.jdbc.pool.DataSource}, or an + * {@link XADataSource} when the reference names {@code javax.sql.XADataSource} + * or the pool's own XA class. * </p> * * @param obj The possibly null object containing location or reference information that can be used in @@ -396,14 +388,26 @@ public class DataSourceFactory implements ObjectFactory { * <code>null</code> if <code>name</code> is relative to the default initial context * @param environment The possibly null environment that is used in creating this object * + * @return the configured data source, or <code>null</code> if <code>obj</code> is not a + * <code>Reference</code> or names a class this factory does not build (the + * latter is logged) + * + * @throws IllegalArgumentException if an attribute carries a value its property cannot + * take: an unusable boolean, numeric or transaction-isolation value, or a + * {@code dataSource} attribute holding a String or another connection pool + * @throws NamingException if the name given by the {@code dataSourceJNDI} attribute does + * not resolve to a data source that can back the pool + * * @exception Exception if an exception occurs creating the instance */ @Override public Object getObjectInstance(Object obj, Name name, Context nameCtx, Hashtable<?, ?> environment) throws Exception { - // We only know how to deal with <code>javax.naming.Reference</code>s - // that specify a class name of "javax.sql.DataSource" + // The reference must name one of the types this factory builds: the + // two documented interface names and the pool's own concrete classes + // (the non-XA one has always been accepted; the XA one is exactly + // what the factory news for an XA reference). if ((obj == null) || !(obj instanceof Reference)) { return null; } @@ -420,30 +424,51 @@ public class DataSourceFactory implements ObjectFactory { if (org.apache.tomcat.jdbc.pool.DataSource.class.getName().equals(ref.getClassName())) { ok = true; } + if (org.apache.tomcat.jdbc.pool.XADataSource.class.getName().equals(ref.getClassName())) { + ok = true; + XA = true; + } if (!ok) { log.warn(MessageFactory.get("dataSourceFactory.invalidClassName", ref.getClassName())); return null; } - Properties properties = new Properties(); - DataSource dataSourceObject = null; + Object dataSourceObject = null; for (int i = 0; i < ALL_PROPERTIES.length; i++) { String propertyName = ALL_PROPERTIES[i]; RefAddr ra = ref.get(propertyName); if (ra != null) { Object content = ra.getContent(); if (content == null) { - // RefAddr.getContent() is nullable, and a property with no - // content simply has no value to apply + // RefAddr.getContent() is nullable: a property carrying no + // content has no value to apply. The name is recognised, + // so note the skip where an operator wondering why a + // configured attribute did not take effect will look. + if (log.isDebugEnabled()) { + log.debug(MessageFactory.get("dataSourceFactory.nullContentProperty", propertyName)); + } 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; + if (PROP_DATASOURCE.equals(propertyName)) { + if (content instanceof DataSourceProxy) { + // A pool cannot wrap another pool; reported with the + // attribute and the object's class, the context the + // layered-pool rejection deeper down cannot supply - + // the lookup route already diagnoses this binding with + // the same details. + throw new IllegalArgumentException(MessageFactory.get( + "dataSourceFactory.cannotSetDataSourceLayered", + content.getClass().getName())); + } + if (isSupportedDataSourceObject(content)) { + // A live data source cannot travel through Properties + // as a string, so keep the object and hand it to the + // pool below + dataSourceObject = content; + continue; + } } properties.setProperty(propertyName, content.toString()); } @@ -452,11 +477,16 @@ public class DataSourceFactory implements ObjectFactory { // Whatever the lookup above did not recognize is never read, so the // operator hears about it here instead of watching the pool come up // with defaults. Names the JNDI container itself uses to build the - // reference are not pool properties and stay unmentioned. + // reference are not pool properties and stay unmentioned. The set is + // read from ALL_PROPERTIES at lookup time rather than snapshotted at + // class-initialisation: the array is protected so a subclass can + // extend it, and a snapshot would leave the loop reading one list + // while this sweep judged against another. + Set<String> knownPropertyNames = new HashSet<>(Arrays.asList(ALL_PROPERTIES)); Enumeration<RefAddr> addresses = ref.getAll(); while (addresses.hasMoreElements()) { String addressName = addresses.nextElement().getType(); - if (!ALL_PROPERTY_NAMES.contains(addressName) + if (!knownPropertyNames.contains(addressName) && !RESERVED_JNDI_ADDRESS_NAMES.contains(addressName)) { log.warn(MessageFactory.get("dataSourceFactory.invalidProperty", addressName)); } @@ -471,12 +501,56 @@ public class DataSourceFactory implements ObjectFactory { return createDataSource(properties, nameCtx, XA, dataSourceObject); } + /** + * Whether content carried under the {@code dataSource} attribute is an + * object the pool can build its connections from. The pool accepts the + * three JDBC data source types its connect path understands + * ({@code javax.sql.DataSource}, {@code javax.sql.XADataSource} and + * {@code javax.sql.ConnectionPoolDataSource}, see + * {@link PooledConnection#connectUsingDataSource()}); only the first is a + * {@code javax.sql.DataSource}, so testing for that one type alone would + * stringify an XA or pooled data source and then reject the string. + * + * @param content the {@code RefAddr} content of a {@code dataSource} attribute + * + * @return {@code true} if the object can back the pool + */ + private static boolean isSupportedDataSourceObject(Object content) { + return content instanceof DataSource || content instanceof javax.sql.XADataSource + || content instanceof javax.sql.ConnectionPoolDataSource; + } + /** * Parse the given properties into a PoolConfiguration. * + * <p> + * A blank attribute value - empty or whitespace only, the shape a cleared + * template attribute yields - leaves the property at its + * {@link PoolProperties} default. A value the property cannot take fails the + * parse naming the attribute, rather than being silently dropped the way + * unparsed values used to be. Numeric, boolean and transaction-isolation + * attributes apply their trimmed value, and so does the lookup of the name + * given by {@code dataSourceJNDI}: whitespace around a number, a keyword or a + * class or JNDI name is always noise. Other string values are stored verbatim + * - a catalog, a validation query or a URL can legitimately carry + * surrounding whitespace. + * </p> + * + * <p> + * Two attributes are not plain property assignments: {@code connectionProperties} + * is resolved into the driver properties through + * {@link PoolProperties#setConnectionProperties(String)}, and + * {@code username}/{@code password} are merged into that same driver property + * set afterwards, so credentials configured as their own attributes override a + * {@code user} or {@code password} pair inside {@code connectionProperties}. + * </p> + * * @param properties The properties to parse * * @return The configured PoolConfiguration + * + * @throws IllegalArgumentException if an attribute carries a value its property + * cannot take */ public static PoolConfiguration parsePoolProperties(Properties properties) { PoolConfiguration poolProperties = new PoolProperties(); @@ -500,63 +574,95 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_DEFAULTTRANSACTIONISOLATION); if (value != null) { - int level = UNKNOWN_TRANSACTIONISOLATION; - if ("NONE".equalsIgnoreCase(value)) { - level = Connection.TRANSACTION_NONE; - } else if ("READ_COMMITTED".equalsIgnoreCase(value)) { - level = Connection.TRANSACTION_READ_COMMITTED; - } else if ("READ_UNCOMMITTED".equalsIgnoreCase(value)) { - level = Connection.TRANSACTION_READ_UNCOMMITTED; - } else if ("REPEATABLE_READ".equalsIgnoreCase(value)) { - level = Connection.TRANSACTION_REPEATABLE_READ; - } else if ("SERIALIZABLE".equalsIgnoreCase(value)) { - level = Connection.TRANSACTION_SERIALIZABLE; - } else { - try { - level = Integer.parseInt(value); - } catch (NumberFormatException e) { - System.err.println("Could not parse defaultTransactionIsolation: " + value); - System.err.println("WARNING: defaultTransactionIsolation not set"); - System.err.println("using default value of database driver"); - level = UNKNOWN_TRANSACTIONISOLATION; + String trimmed = value.trim(); + if (!trimmed.isEmpty()) { + // Unset means the pool stays in the "use the driver default" + // state it comes with. + int level; + if ("NONE".equalsIgnoreCase(trimmed)) { + level = Connection.TRANSACTION_NONE; + } else if ("READ_COMMITTED".equalsIgnoreCase(trimmed)) { + level = Connection.TRANSACTION_READ_COMMITTED; + } else if ("READ_UNCOMMITTED".equalsIgnoreCase(trimmed)) { + level = Connection.TRANSACTION_READ_UNCOMMITTED; + } else if ("REPEATABLE_READ".equalsIgnoreCase(trimmed)) { + level = Connection.TRANSACTION_REPEATABLE_READ; + } else if ("SERIALIZABLE".equalsIgnoreCase(trimmed)) { + level = Connection.TRANSACTION_SERIALIZABLE; + } else { + try { + level = Integer.parseInt(trimmed); + } catch (NumberFormatException e) { + // An unusable value fails the resource naming the + // property, instead of silently dropping the + // configured isolation level. + throw new IllegalArgumentException(MessageFactory.get( + "dataSourceFactory.invalidTransactionIsolation", trimmed, + PROP_DEFAULTTRANSACTIONISOLATION), e); + } } + poolProperties.setDefaultTransactionIsolation(level); } - poolProperties.setDefaultTransactionIsolation(level); } value = properties.getProperty(PROP_DEFAULTCATALOG); - if (value != null) { + if (value != null && !value.trim().isEmpty()) { + // A stored empty value would be pushed into every physical + // connection as setCatalog("") and whether that is inert or fails + // is driver-dependent. The value itself is kept untrimmed - a + // catalog name is a database identifier, which may carry + // whitespace the way a JNDI name cannot. poolProperties.setDefaultCatalog(value); } value = properties.getProperty(PROP_DRIVERCLASSNAME); - if (value != null) { - poolProperties.setDriverClassName(value); + if (value != null && !value.trim().isEmpty()) { + // The connect path's DriverManager fallback is keyed on the + // property being null, so a stored empty class name would defeat + // the fallback and abort the pool loading the empty class. The + // trimmed value is applied: a Java class name can never carry + // meaningful surrounding whitespace. + poolProperties.setDriverClassName(value.trim()); } value = properties.getProperty(PROP_MAXACTIVE); if (value != null) { - poolProperties.setMaxActive(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_MAXACTIVE, value); + if (parsed != null) { + poolProperties.setMaxActive(parsed.intValue()); + } } value = properties.getProperty(PROP_MAXIDLE); if (value != null) { - poolProperties.setMaxIdle(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_MAXIDLE, value); + if (parsed != null) { + poolProperties.setMaxIdle(parsed.intValue()); + } } value = properties.getProperty(PROP_MINIDLE); if (value != null) { - poolProperties.setMinIdle(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_MINIDLE, value); + if (parsed != null) { + poolProperties.setMinIdle(parsed.intValue()); + } } value = properties.getProperty(PROP_INITIALSIZE); if (value != null) { - poolProperties.setInitialSize(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_INITIALSIZE, value); + if (parsed != null) { + poolProperties.setInitialSize(parsed.intValue()); + } } value = properties.getProperty(PROP_MAXWAIT); if (value != null) { - poolProperties.setMaxWait(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_MAXWAIT, value); + if (parsed != null) { + poolProperties.setMaxWait(parsed.intValue()); + } } value = properties.getProperty(PROP_TESTONBORROW); @@ -585,18 +691,27 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_TIMEBETWEENEVICTIONRUNSMILLIS); if (value != null) { - poolProperties.setTimeBetweenEvictionRunsMillis(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_TIMEBETWEENEVICTIONRUNSMILLIS, value); + if (parsed != null) { + poolProperties.setTimeBetweenEvictionRunsMillis(parsed.intValue()); + } } value = properties.getProperty(PROP_NUMTESTSPEREVICTIONRUN); if (value != null) { log.warn(MessageFactory.get("dataSourceFactory.invalidSetting", PROP_NUMTESTSPEREVICTIONRUN)); - poolProperties.setNumTestsPerEvictionRun(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_NUMTESTSPEREVICTIONRUN, value); + if (parsed != null) { + poolProperties.setNumTestsPerEvictionRun(parsed.intValue()); + } } value = properties.getProperty(PROP_MINEVICTABLEIDLETIMEMILLIS); if (value != null) { - poolProperties.setMinEvictableIdleTimeMillis(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_MINEVICTABLEIDLETIMEMILLIS, value); + if (parsed != null) { + poolProperties.setMinEvictableIdleTimeMillis(parsed.intValue()); + } } value = properties.getProperty(PROP_TESTWHILEIDLE); @@ -623,13 +738,23 @@ public class DataSourceFactory implements ObjectFactory { } value = properties.getProperty(PROP_VALIDATIONQUERY); - if (value != null) { + if (value != null && !value.trim().isEmpty()) { + // The way to express "no validation query" is the property being + // unset, which routes validation to Connection.isValid(); a + // stored empty string instead runs the empty string as SQL on + // every validation - a no-op on forgiving drivers and a failed + // validation, session discard and reconnect on stricter ones. The + // query itself is kept untrimmed; whitespace around real SQL is + // the driver's to chew. poolProperties.setValidationQuery(value); } value = properties.getProperty(PROP_VALIDATIONQUERY_TIMEOUT); if (value != null) { - poolProperties.setValidationQueryTimeout(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_VALIDATIONQUERY_TIMEOUT, value); + if (parsed != null) { + poolProperties.setValidationQueryTimeout(parsed.intValue()); + } } value = properties.getProperty(PROP_VALIDATOR_CLASS_NAME); @@ -639,7 +764,10 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_VALIDATIONINTERVAL); if (value != null) { - poolProperties.setValidationInterval(Long.parseLong(value)); + Long parsed = parseNumber(PROP_VALIDATIONINTERVAL, value); + if (parsed != null) { + poolProperties.setValidationInterval(parsed.longValue()); + } } value = properties.getProperty(PROP_ACCESSTOUNDERLYINGCONNECTIONALLOWED); @@ -661,7 +789,10 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_REMOVEABANDONEDTIMEOUT); if (value != null) { - poolProperties.setRemoveAbandonedTimeout(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_REMOVEABANDONEDTIMEOUT, value); + if (parsed != null) { + poolProperties.setRemoveAbandonedTimeout(parsed.intValue()); + } } value = properties.getProperty(PROP_LOGABANDONED); @@ -684,10 +815,9 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_CONNECTIONPROPERTIES); if (value != null) { - // Use the canonical setter so that the configured string also - // round-trips through getConnectionProperties(); it parses the - // pairs into getDbProperties() with the very same parser that - // getProperties(value) below used to apply. + // The canonical setter parses the pairs into the driver + // properties and keeps the string round-tripping through + // getConnectionProperties(). poolProperties.setConnectionProperties(value); } else { poolProperties.setDbProperties(new Properties()); @@ -706,7 +836,10 @@ public class DataSourceFactory implements ObjectFactory { } value = properties.getProperty(PROP_INTERCEPTORS); - if (value != null) { + if (value != null && !value.trim().isEmpty()) { + // Storing the empty string would leave the property non-null, and + // the pool treats that as a configured chain whose only segment is + // the empty one. poolProperties.setJdbcInterceptors(value); } @@ -735,46 +868,67 @@ public class DataSourceFactory implements ObjectFactory { } value = properties.getProperty(OBJECT_NAME); - if (value != null) { + if (value != null && !value.trim().isEmpty()) { + // An empty attribute would be quoted into an empty pool name, + // giving the JMX object name an empty name component where the + // property being unset lets the pool name itself. The configured + // name is stored verbatim - it is an identifier and can carry + // meaningful whitespace, which is also why it is quoted. poolProperties.setName(ObjectName.quote(value)); } value = properties.getProperty(PROP_ABANDONWHENPERCENTAGEFULL); if (value != null) { - poolProperties.setAbandonWhenPercentageFull(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_ABANDONWHENPERCENTAGEFULL, value); + if (parsed != null) { + poolProperties.setAbandonWhenPercentageFull(parsed.intValue()); + } } value = properties.getProperty(PROP_MAXAGE); if (value != null) { - poolProperties.setMaxAge(Long.parseLong(value)); + Long parsed = parseNumber(PROP_MAXAGE, value); + if (parsed != null) { + poolProperties.setMaxAge(parsed.longValue()); + } } - value = properties.getProperty(PROP_USE_CON_LOCK); + value = properties.getProperty(PROP_USE_LOCK); if (value != null) { - Boolean b = parseBoolean(PROP_USE_CON_LOCK, value); + Boolean b = parseBoolean(PROP_USE_LOCK, value); if (b != null) { poolProperties.setUseLock(b.booleanValue()); } } value = properties.getProperty(PROP_DATASOURCE); - if (value != null) { + if (value != null && !value.trim().isEmpty()) { // 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. + // point: getObjectInstance hands a live data source object + // carried under the attribute (DataSource, XADataSource or + // ConnectionPoolDataSource) straight to the pool instead of + // stringifying it. A string cannot be turned into a data source + // here. throw new IllegalArgumentException(MessageFactory.get("dataSourceFactory.cannotSetDataSourceString", value)); } value = properties.getProperty(PROP_DATASOURCE_JNDI); if (value != null) { - poolProperties.setDataSourceJNDI(value); + String trimmed = value.trim(); + if (!trimmed.isEmpty()) { + // A stored blank name would be one no context can resolve, + // failing the whole resource on the lookup. + poolProperties.setDataSourceJNDI(trimmed); + } } value = properties.getProperty(PROP_SUSPECT_TIMEOUT); if (value != null) { - poolProperties.setSuspectTimeout(Integer.parseInt(value)); + Integer parsed = parseInteger(PROP_SUSPECT_TIMEOUT, value); + if (parsed != null) { + poolProperties.setSuspectTimeout(parsed.intValue()); + } } value = properties.getProperty(PROP_ALTERNATE_USERNAME_ALLOWED); @@ -870,6 +1024,62 @@ public class DataSourceFactory implements ObjectFactory { throw new IllegalArgumentException(MessageFactory.get("dataSourceFactory.invalidBoolean", value, name)); } + /** + * Parses the value configured for a numeric attribute. Like + * {@link #parseBoolean(String, String)} the value tolerates surrounding + * whitespace, an empty value means "leave the property unset" rather than + * failing the parse, and a value that cannot be parsed fails the + * configuration naming the property and quoting the value instead of + * surfacing as a bare {@link NumberFormatException} about an anonymous + * input string. + * + * @param name the property name, for the error message + * @param value the configured value, never {@code null} + * + * @return the parsed value, or {@code null} when the value is empty and + * the property is therefore to be left unset + * + * @throws IllegalArgumentException if the value is not a whole number + */ + private static Long parseNumber(String name, String value) { + String trimmed = value.trim(); + if (trimmed.isEmpty()) { + return null; + } + try { + return Long.valueOf(trimmed); + } catch (NumberFormatException e) { + throw new IllegalArgumentException( + MessageFactory.get("dataSourceFactory.invalidNumber", trimmed, name), e); + } + } + + /** + * Parses the value configured for a numeric attribute whose setter takes + * an {@code int}. + * + * @param name the property name, for the error message + * @param value the configured value, never {@code null} + * + * @return the parsed value, or {@code null} when the value is empty and + * the property is therefore to be left unset + * + * @throws IllegalArgumentException if the value is not a whole number, or + * is outside the range an {@code int} + * can hold + */ + private static Integer parseInteger(String name, String value) { + Long parsed = parseNumber(name, value); + if (parsed == null) { + return null; + } + if (parsed.longValue() < Integer.MIN_VALUE || parsed.longValue() > Integer.MAX_VALUE) { + throw new IllegalArgumentException( + MessageFactory.get("dataSourceFactory.numberOutOfRange", value, name)); + } + return Integer.valueOf(parsed.intValue()); + } + /** * Creates and configures a {@link DataSource} instance based on the given properties. * @@ -904,20 +1114,22 @@ public class DataSourceFactory implements ObjectFactory { * <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. + * to the factory directly. The object is one of the data source types the pool's connect path accepts + * ({@code javax.sql.DataSource}, {@code javax.sql.XADataSource} or + * {@code javax.sql.ConnectionPoolDataSource}), not necessarily a {@code javax.sql.DataSource}. * </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} + * @param dataSourceObject A live data source object 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 { + Object dataSourceObject) throws Exception { PoolConfiguration poolProperties = parsePoolProperties(properties); if (dataSourceObject != null) { poolProperties.setDataSource(dataSourceObject); @@ -927,9 +1139,7 @@ public class DataSourceFactory implements ObjectFactory { } org.apache.tomcat.jdbc.pool.DataSource dataSource = XA ? new XADataSource(poolProperties) : new org.apache.tomcat.jdbc.pool.DataSource(poolProperties); - // Initialise the pool itself dataSource.createPool(); - // Return the configured DataSource instance return dataSource; } @@ -939,8 +1149,9 @@ public class DataSourceFactory implements ObjectFactory { * @param context The JNDI context * @param poolProperties The pool configuration * - * @throws NamingException if the name configured through the dataSourceJNDI property cannot be resolved. The name - * was configured specifically to supply the connections for the pool, so a pool without + * @throws NamingException if the name configured through the dataSourceJNDI property cannot be resolved, or + * resolves to an object the pool cannot create connections from. The name was + * configured specifically to supply the connections for the pool, so a pool without * that data source is not a usable fallback and the failure is reported instead of * being deferred to the first connection request. */ @@ -950,13 +1161,15 @@ public class DataSourceFactory implements ObjectFactory { if (context != null) { jndiDS = context.lookup(poolProperties.getDataSourceJNDI()); } else { - log.warn(MessageFactory.get("dataSourceFactory.jndiContextNull")); + // Not a fault: a bare Reference lookup outside a container + // arrives with no local context, and the InitialContext + // fallback below is written for exactly that. + log.debug(MessageFactory.get("dataSourceFactory.jndiContextNull")); } } catch (NamingException e) { if (log.isDebugEnabled()) { - log.debug( - "The name \"" + poolProperties.getDataSourceJNDI() + "\" cannot be found in the local context.", - e); + log.debug(MessageFactory.get("dataSourceFactory.jndiLocalNameNotFound", + poolProperties.getDataSourceJNDI()), e); } } NamingException initialContextFailure = null; @@ -965,12 +1178,37 @@ public class DataSourceFactory implements ObjectFactory { context = new InitialContext(); jndiDS = context.lookup(poolProperties.getDataSourceJNDI()); } catch (NamingException e) { + // Not warned here: this failure always ends in the thrown + // jndiLookupFailed below, which names the same name and + // carries this exception as its cause, and the caller logs + // that - warning as well only made the one failed resource + // read as two. initialContextFailure = e; - log.warn(MessageFactory.get("dataSourceFactory.jndiNameNotFound", - poolProperties.getDataSourceJNDI()), e); } } if (jndiDS != null) { + if (jndiDS instanceof DataSourceProxy) { + // setDataSource's own rejection of layered pools, reported as + // this method's contract promises and with the context the + // rejection alone lacks: the attribute and the name that + // resolved to the other pool. + throw new NamingException(MessageFactory.get( + "dataSourceFactory.jndiLayeredPool", + poolProperties.getDataSourceJNDI(), + jndiDS.getClass().getName())); + } + if (!isSupportedDataSourceObject(jndiDS)) { + // The same requirement the object-valued dataSource attribute + // enforces: the name was configured to supply the pool's + // connections, and a binding the connect path cannot use must + // fail here naming the attribute and the name, rather than + // surfacing at connect time as a generic error about an + // unknown class. + throw new NamingException(MessageFactory.get( + "dataSourceFactory.jndiNotDataSource", + poolProperties.getDataSourceJNDI(), + jndiDS.getClass().getName())); + } poolProperties.setDataSource(jndiDS); return; } @@ -986,6 +1224,14 @@ public class DataSourceFactory implements ObjectFactory { /** * Parse properties from the string. Format of the string must be [propertyName=property;]*. * + * <p> + * Kept as a protected extension point for subclasses that parse property + * strings of their own; this factory no longer calls it, as its + * {@code connectionProperties} attribute goes through the canonical + * {@link PoolProperties#setConnectionProperties(String)}, which parses with the + * same delegate used here. + * </p> + * * @param propText The properties string * * @return the properties diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index c1ef63bc69..5340b6ce1c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -20,9 +20,11 @@ import java.io.IOException; import java.io.Serializable; import java.io.StringReader; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Properties; @@ -454,18 +456,38 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl interceptors = new InterceptorDefinition[0]; } else { String[] interceptorValues = interceptorList.split(";"); - InterceptorDefinition[] definitions = new InterceptorDefinition[interceptorValues.length + 1]; + List<InterceptorDefinition> definitions = + new ArrayList<>(interceptorValues.length + 1); // Always add the trap interceptor to the mix - definitions[0] = new InterceptorDefinition(TrapException.class); + definitions.add(new InterceptorDefinition(TrapException.class)); for (int i = 0; i < interceptorValues.length; i++) { int propIndex = interceptorValues[i].indexOf('('); int endIndex = interceptorValues[i].indexOf(')'); + String name; + String propsAsString = null; if (propIndex < 0 || endIndex < 0 || endIndex <= propIndex) { - definitions[i + 1] = new InterceptorDefinition(interceptorValues[i].trim()); + name = interceptorValues[i].trim(); } else { - String name = interceptorValues[i].substring(0, propIndex).trim(); - definitions[i + 1] = new InterceptorDefinition(name); - String propsAsString = interceptorValues[i].substring(propIndex + 1, endIndex); + name = interceptorValues[i].substring(0, propIndex).trim(); + propsAsString = interceptorValues[i].substring(propIndex + 1, endIndex); + } + if (name.isEmpty()) { + // The name is empty when a leading or doubled + // separator leaves an empty segment, or when a + // segment opens straight on "(": there is no + // interceptor to load, and a definition without a + // class name fails the whole pool start later, on + // the generated "...pool.interceptor." class, with a + // message naming nothing. split() already drops a + // redundant trailing separator and the property parse + // below tolerates a redundant separator inside the + // parentheses: skipping the empty name makes a + // redundant separator cosmetic throughout the syntax + // rather than fatal wherever split() kept it. + continue; + } + InterceptorDefinition definition = new InterceptorDefinition(name); + if (propsAsString != null) { String[] props = propsAsString.split(","); for (int j = 0; j < props.length; j++) { String prop = props[j]; @@ -484,11 +506,12 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } String propName = prop.substring(0, pidx).trim(); String propValue = prop.substring(pidx + 1).trim(); - definitions[i + 1].addProperty(new InterceptorProperty(propName, propValue)); + definition.addProperty(new InterceptorProperty(propName, propValue)); } } + definitions.add(definition); } - interceptors = definitions; + interceptors = definitions.toArray(new InterceptorDefinition[0]); } } return interceptors; @@ -509,7 +532,10 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl * properties to the driver instead of leaving them merged in. Entries the * string never contributed, such as the {@code user} and {@code password} * injected by {@link DataSourceFactory#parsePoolProperties(Properties)} or - * mirrored in by {@link DataSourceProxy}, are kept. A configuration whose + * mirrored in by {@link DataSourceProxy}, are kept, and while a + * {@code username}/{@code password} field is set its entry holds the + * field's value even when this string names that key, the rule the + * credential setters establish. A configuration whose * database properties are {@code null} only stores the raw string, as there * is no property collection to parse into. The remove and re-add run inside * this one synchronized method; an unsynchronised reader of the database @@ -544,6 +570,19 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl dbProps.setProperty(key, parsed.getProperty(key)); keys.add(key); } + // The credential setters' rule outlives the string: while a credential + // field is set, its dbProperties entry holds the field's value, even + // when this string named the key itself - DataSourceFactory injects the + // configured username/password over the string's pairs, so a + // reconfigure that drops the keys from the string must not strip + // credentials the configuration still carries. With the field unset the + // entry is the string's own again, exactly as after setPassword(null). + if (username != null) { + dbProps.setProperty(PoolUtilities.PROP_USER, username); + } + if (password != null) { + dbProps.setProperty(PoolUtilities.PROP_PASSWORD, password); + } connectionPropertyKeys = keys; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 8e937701fc..591d47308b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -86,14 +86,21 @@ dataSource.unableToCreateObjectName=Unable to create object name for JDBC pool. dataSource.unableToRegisterMBean=Unable to register JDBC pool MBean. dataSource.unableToUnregisterMBean=Unable to unregister JDBC pool MBean [{0}]. The registration handle was already given up, so the close or callback that asked for this release continues. -dataSourceFactory.cannotSetDataSourceString=Can''t set dataSource property as a string "{0}". 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. +dataSourceFactory.cannotSetDataSourceLayered=Can''t set dataSource property to another connection pool ({0}): a pool cannot wrap another pool. Carry the JDBC driver''s data source object under the dataSource attribute, or use the dataSourceJNDI attribute to name one bound in JNDI. +dataSourceFactory.cannotSetDataSourceString=Can''t set dataSource property as a string "{0}". Either carry the data source object itself under the dataSource attribute of the Reference - a javax.sql.DataSource, javax.sql.XADataSource or javax.sql.ConnectionPoolDataSource - or use the dataSourceJNDI attribute to name a data source bound in JNDI. dataSourceFactory.invalidBoolean=Invalid boolean value ''{0}'' for property {1} dataSourceFactory.invalidClassName={0} is not a valid class name/type for this JNDI factory. +dataSourceFactory.invalidNumber=Invalid numeric value ''{0}'' for property {1}: expected a whole number dataSourceFactory.invalidProperty={0} is not a valid property, it will be ignored. dataSourceFactory.invalidSetting={0} is not a valid setting, it will have no effect. +dataSourceFactory.invalidTransactionIsolation=Invalid value ''{0}'' for property {1}: expected NONE, READ_UNCOMMITTED, READ_COMMITTED, REPEATABLE_READ, SERIALIZABLE or the numeric transaction isolation level of java.sql.Connection dataSourceFactory.jndiContextNull=dataSourceJNDI property is configured, but local JNDI context is null. +dataSourceFactory.jndiLayeredPool=The name [{0}] resolved through the ''dataSourceJNDI'' attribute to another connection pool ({1}): a pool cannot wrap another pool. Point the name at a data source provided by the JDBC driver. +dataSourceFactory.jndiLocalNameNotFound=The name "{0}" cannot be found in the local context. dataSourceFactory.jndiLookupFailed=Unable to create the connection pool: the name [{0}] configured through the ''dataSourceJNDI'' attribute was not found in the local JNDI context nor in the InitialContext. The pool is built around the data source that name refers to; check that the name is correct and that the data source is bound and visible to the application before the pool is created. -dataSourceFactory.jndiNameNotFound=The name "{0}" cannot be found in the InitialContext. +dataSourceFactory.jndiNotDataSource=The name [{0}] resolved through the ''dataSourceJNDI'' attribute to an object of class {1}, which the pool cannot create connections from. The name must be bound to a javax.sql.DataSource, a javax.sql.XADataSource or a javax.sql.ConnectionPoolDataSource. +dataSourceFactory.nullContentProperty=The {0} attribute of the JNDI reference carries no content; the property is left unset. +dataSourceFactory.numberOutOfRange=Invalid numeric value ''{0}'' for property {1}: the value is outside the range the property accepts dataSourceProxy.alternateUsernameIgnored=alternateUsernameAllowed is set to false so the provided user name [{0}] and associated password have been ignored dataSourceProxy.connectionNotXA=Connection from pool does not implement javax.sql.XAConnection diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryJndiLookup.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryJndiLookup.java index 8a2698ff8e..1e00065b55 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryJndiLookup.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryJndiLookup.java @@ -177,6 +177,103 @@ class TestDataSourceFactoryJndiLookup extends JdbcPoolBaseTest { "the pool must establish connections through the looked up data source"); } + @Test + void dataSourceJNDIBoundToSomethingElseFailsTheFactoryNamingTheClass() { + // The sibling dataSource attribute validates its object against the + // types the connect path understands; the lookup route must hold the + // bound object to the same requirement. Pre-fix any binding reached + // the pool and only surfaced at connect time as a generic error about + // an unknown class, if the initial size deferred the first connect at + // all. + Reference ref = backingDataSourceReference("0"); + Context context = new ContextStub() { + @Override + public Object lookup(String name) { + return MISSING_JNDI_NAME.equals(name) ? "not-a-data-source" : null; + } + + @Override + public Object lookup(Name name) { + return lookup(name.toString()); + } + }; + + NamingException e = Assertions.assertThrows(NamingException.class, + () -> new DataSourceFactory().getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), context, null), + "a name bound to a class the pool cannot connect from must fail the factory call"); + Assertions.assertTrue(e.getMessage().contains(MISSING_JNDI_NAME), + "the failure must name the misbound name, was: " + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("dataSourceJNDI"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains(String.class.getName()), + "the failure must name the class the name resolved to, was: " + + e.getMessage()); + } + + @Test + void dataSourceJNDIBoundToAnotherPoolFailsWithANamingException() { + // A binding of another tomcat-jdbc pool is rejected by setDataSource + // as a layered pool - but performJNDILookup promises NamingException, + // and the bare IllegalArgumentException carried neither the attribute + // nor the name that resolved to the other pool. + Reference ref = backingDataSourceReference("0"); + org.apache.tomcat.jdbc.pool.DataSource otherPool = + new org.apache.tomcat.jdbc.pool.DataSource(); + Context context = new ContextStub() { + @Override + public Object lookup(String name) { + return MISSING_JNDI_NAME.equals(name) ? otherPool : null; + } + + @Override + public Object lookup(Name name) { + return lookup(name.toString()); + } + }; + + NamingException e = Assertions.assertThrows(NamingException.class, + () -> new DataSourceFactory().getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), context, null), + "a layered-pool binding must fail with the method's declared" + + " exception type, not an unchecked rejection"); + Assertions.assertTrue(e.getMessage().contains(MISSING_JNDI_NAME), + "the failure must name the misbound name, was: " + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("dataSourceJNDI"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + } + + @Test + void blankDataSourceJNDIAttributeLeavesThePropertyUnset() throws Exception { + // A blank value means "not configured", as it does for the boolean, + // numeric and isolation attributes of the same factory. Pre-fix the + // blank was stored and looked up as a name, and the resource died on + // a NamingException about the name [] even though the pool had a + // perfectly usable driver/URL configuration. + 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")); + 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("dataSourceJNDI", " ")); + + DataSource ds = manage((DataSource) new DataSourceFactory() + .getObjectInstance(ref, new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertNull(ds.getPoolProperties().getDataSourceJNDI(), + "a blank dataSourceJNDI must leave the property unset"); + try (java.sql.Connection c = ds.getConnection(); + java.sql.ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the pool must still work from its driver/URL configuration"); + } + } + /** * A JNDI context in which nothing is bound: every operation except * {@code lookup} is unsupported, and no {@code lookup} can succeed. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryNoOpPropertyWarnings.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryNoOpPropertyWarnings.java index c7a027baec..e9800dfb37 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryNoOpPropertyWarnings.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryNoOpPropertyWarnings.java @@ -131,17 +131,16 @@ class TestDataSourceFactoryNoOpPropertyWarnings { /** * A factory that stops where the real one parses the properties, so the * full JNDI route can be exercised without creating a pool behind it. The - * {@code dataSourceObject} parameter type is deliberately the fully - * qualified {@code javax.sql.DataSource} of the overridden signature: an - * override that narrows it to the pool's own class would be an overload - * the factory never calls. + * {@code dataSourceObject} parameter type is deliberately the widened + * {@code Object} of the overridden signature: an override that narrows it + * would be an overload the factory never calls. */ private static final class ParsingFactory extends DataSourceFactory { private PoolConfiguration parsed; @Override protected javax.sql.DataSource createDataSource(Properties properties, Context context, boolean xa, - javax.sql.DataSource dataSourceObject) { + Object dataSourceObject) { // exactly what the real implementation does first parsed = DataSourceFactory.parsePoolProperties(properties); return null; @@ -208,12 +207,18 @@ class TestDataSourceFactoryNoOpPropertyWarnings { properties.setProperty("numTestsPerEvictionRun", "not-a-number"); try (FactoryLogCapture logs = new FactoryLogCapture()) { - Assertions.assertThrows(NumberFormatException.class, + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, () -> DataSourceFactory.parsePoolProperties(properties), "a value that cannot be parsed must still fail the parse, as it did before"); // the warning reports the property being configured, not the // success of parsing it, so it must precede the parse failure logs.assertWarnedNoEffect("numTestsPerEvictionRun"); + Assertions.assertInstanceOf(NumberFormatException.class, e.getCause(), + "the parse failure must be kept as the cause"); + Assertions.assertTrue(e.getMessage().contains("numTestsPerEvictionRun"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); } } 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 index c02eac4890..bef1e185da 100644 --- 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 @@ -42,8 +42,9 @@ import org.apache.tomcat.jdbc.pool.PoolProperties; * 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()}. + * actual backing object must reach the pool as that object rather than + * as its {@code toString()} - for every backing type the pool's own + * connect path accepts, not only {@code javax.sql.DataSource}. */ class TestDataSourceFactoryRefAddrContent extends JdbcPoolBaseTest { @@ -119,6 +120,65 @@ class TestDataSourceFactoryRefAddrContent extends JdbcPoolBaseTest { "the pool must establish connections through that data source"); } + @Test + void objectValuedXaOnlyDataSourceRefAddrBacksThePool() throws Exception { + // A driver's XA data source, like Oracle's, implements + // javax.sql.XADataSource without being a javax.sql.DataSource. The + // pool's connect path supports exactly that backing, but the + // instanceof test in the factory loop recognised only + // javax.sql.DataSource: the live object was stringified and the + // resource then died on the resulting string with a message telling + // the operator to do what the Reference already did. + Reference ref = new Reference("javax.sql.XADataSource", null, null); + ref.add(new StringRefAddr("jmxEnabled", "false")); + ref.add(new StringRefAddr("initialSize", "1")); + ref.add(new StringRefAddr("minIdle", "0")); + org.h2.jdbcx.JdbcDataSource delegate = new org.h2.jdbcx.JdbcDataSource(); + delegate.setUrl(getUrl()); + delegate.setUser("sa"); + delegate.setPassword(""); + Object xaOnly = singleInterfaceDataSource(delegate, javax.sql.XADataSource.class); + ref.add(new ObjectContentRefAddr("dataSource", xaOnly)); + + Object result = new DataSourceFactory().getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), null, null); + Assertions.assertInstanceOf(org.apache.tomcat.jdbc.pool.XADataSource.class, result, + "a javax.sql.XADataSource reference must yield the pool's XA data source"); + org.apache.tomcat.jdbc.pool.XADataSource ds = + manage((org.apache.tomcat.jdbc.pool.XADataSource) result); + + Assertions.assertSame(xaOnly, 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 XA data source"); + } + + @Test + void objectValuedConnectionPoolDataSourceRefAddrBacksThePool() throws Exception { + // Same as above for the pooled-connections backing: a driver + // data source that is a javax.sql.ConnectionPoolDataSource but not + // a javax.sql.DataSource (PostgreSQL's CPDS sources, for instance) + // must reach the pool as an object, not as its toString(). + Reference ref = poolReference(); + org.h2.jdbcx.JdbcDataSource delegate = new org.h2.jdbcx.JdbcDataSource(); + delegate.setUrl(getUrl()); + delegate.setUser("sa"); + delegate.setPassword(""); + Object cpdsOnly = singleInterfaceDataSource(delegate, + javax.sql.ConnectionPoolDataSource.class); + ref.add(new ObjectContentRefAddr("dataSource", cpdsOnly)); + + DataSource ds = managedPool(new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/TestDS"), null, null)); + + Assertions.assertSame(cpdsOnly, 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 pooled-connections data source"); + } + @Test void objectValuedDataSourceRefAddrTakesPrecedenceOverDataSourceJNDI() throws Exception { @@ -147,6 +207,32 @@ class TestDataSourceFactoryRefAddrContent extends JdbcPoolBaseTest { assertPoolConnects(ds); } + @Test + void objectValuedDataSourceRefAddrCarryingAnotherPoolIsRejectedWithTheAttributeContext() { + // A pool cannot wrap another pool. Pre-fix the object passed the + // factory's own acceptance check and was rejected deeper down by + // PoolProperties with a bare "Layered pools are not allowed." - + // neither the attribute at fault nor the object's class was named, + // unlike every other failure this factory reports (and unlike the + // dataSourceJNDI route's diagnosis of the same binding). + Reference ref = poolReference(); + org.apache.tomcat.jdbc.pool.DataSource otherPool = + new org.apache.tomcat.jdbc.pool.DataSource(); + ref.add(new ObjectContentRefAddr("dataSource", otherPool)); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new DataSourceFactory().getObjectInstance(ref, + new CompositeName("jdbc/TestDS"), null, null)); + Assertions.assertTrue(e.getMessage().contains("dataSource"), + "the rejection must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue( + e.getMessage().contains(otherPool.getClass().getName()), + "the rejection must name the class the attribute carried, was: " + + e.getMessage()); + } + @Test void stringValuedDataSourceRefAddrIsRejectedWithAnActionableMessage() { // A genuinely string-valued dataSource attribute still cannot become @@ -271,6 +357,35 @@ class TestDataSourceFactoryRefAddrContent extends JdbcPoolBaseTest { } } + /* + * A JDBC data source exposed through a single interface, as the drivers + * that give their XA or pooled data source only that one type do. Every + * interface call is delegated to the given real data source, which must + * implement that interface. + */ + private static Object singleInterfaceDataSource(final Object delegate, + Class<?> exposedInterface) { + return Proxy.newProxyInstance( + TestDataSourceFactoryRefAddrContent.class.getClassLoader(), + new Class<?>[] { exposedInterface }, + (proxy, method, args) -> { + switch (method.getName()) { + case "toString": + return "SingleInterfaceDataSource"; + case "hashCode": + return System.identityHashCode(proxy); + case "equals": + return proxy == args[0]; + default: + try { + return method.invoke(delegate, args); + } catch (java.lang.reflect.InvocationTargetException e) { + throw e.getCause(); + } + } + }); + } + /* * A JNDI context whose lookup returns the given object for any name, * standing in for a bound data source. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryUnknownPropertyWarnings.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryUnknownPropertyWarnings.java index 273aedb0ad..7435203793 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryUnknownPropertyWarnings.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestDataSourceFactoryUnknownPropertyWarnings.java @@ -165,7 +165,7 @@ class TestDataSourceFactoryUnknownPropertyWarnings extends JdbcPoolBaseTest { @Override protected javax.sql.DataSource createDataSource(Properties properties, Context context, boolean xa, - javax.sql.DataSource dataSourceObject) { + Object dataSourceObject) { // exactly what the real implementation does first parsed = DataSourceFactory.parsePoolProperties(properties); return null; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java index 57abf46aee..47ee390bdb 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java @@ -36,6 +36,7 @@ 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.PoolConfiguration; +import org.apache.tomcat.jdbc.pool.PoolProperties; import org.apache.tomcat.jdbc.pool.XADataSource; /** @@ -107,6 +108,23 @@ public class TestNamingFactory extends JdbcPoolBaseTest { } } + @Test + void concreteXaPoolClassNameReferenceProducesXaDataSourceType() throws Exception { + // A resource whose type names the factory's own XA class - the class + // the factory news for every XA reference - must build that XA pool, + // where previously only the sibling non-XA concrete class was + // accepted and this one came back as an unresolvable resource. + Name name = new CompositeName("jdbc/ConcreteXaDS"); + Object result = new DataSourceFactory().getObjectInstance( + dataSourceReference("org.apache.tomcat.jdbc.pool.XADataSource"), + name, null, null); + Assertions.assertInstanceOf(XADataSource.class, result, + "the concrete XA class name must yield the pool's XA type"); + if (result instanceof DataSource) { + manage((DataSource) result); + } + } + /* * Boolean attributes are applied strictly: only "true" and "false", * ignoring case and surrounding whitespace, configure the property; @@ -228,6 +246,299 @@ public class TestNamingFactory extends JdbcPoolBaseTest { "the exact lowercase spelling must keep working"); } + /* + * The defaultTransactionIsolation attribute follows the same strict + * policy: names and the numeric java.sql.Connection constants are + * accepted ignoring surrounding whitespace, an empty attribute means + * "unset" - the "leave the driver default alone" state - and anything + * else fails the parse. Pre-fix an unusable value was silently applied + * as that unset sentinel, so connections ran at the driver's isolation + * level while the only trace was a System.err line claiming the + * attribute had not been set at all. + */ + + @Test + void isolationLevelNamesApplyIgnoringCaseAndSurroundingWhitespace() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultTransactionIsolation", " read_committed "); + Assertions.assertEquals(java.sql.Connection.TRANSACTION_READ_COMMITTED, + DataSourceFactory.parsePoolProperties(attrs) + .getDefaultTransactionIsolation(), + "\" read_committed \" must be accepted, whitespace included"); + + attrs.setProperty("defaultTransactionIsolation", "NONE"); + Assertions.assertEquals(java.sql.Connection.TRANSACTION_NONE, + DataSourceFactory.parsePoolProperties(attrs) + .getDefaultTransactionIsolation(), + "the documented name must still apply"); + } + + @Test + void numericIsolationLevelAppliesIgnoringSurroundingWhitespace() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultTransactionIsolation", " 2 "); + Assertions.assertEquals(2, + DataSourceFactory.parsePoolProperties(attrs) + .getDefaultTransactionIsolation(), + "the numeric form must be accepted, whitespace included"); + } + + @Test + void unusableIsolationLevelFailsTheParseNamingTheProperty() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultTransactionIsolation", "READ_COMMIT"); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> DataSourceFactory.parsePoolProperties(attrs), + "an unusable isolation level must fail the parse instead of " + + "silently leaving the level at the driver default"); + Assertions.assertTrue(e.getMessage().contains("defaultTransactionIsolation"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("READ_COMMIT"), + "the failure must show the offending value, was: " + + e.getMessage()); + Assertions.assertNotNull(e.getCause(), + "the parse failure must be kept as the cause"); + } + + @Test + void emptyIsolationLevelAttributeLeavesThePropertyUnset() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultTransactionIsolation", ""); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertEquals(DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION, + configured.getDefaultTransactionIsolation(), + "an empty attribute must keep the \"leave the driver default " + + "alone\" state"); + } + + /* + * The numeric attributes follow the same strict policy as the booleans: + * surrounding whitespace is ignored, an empty attribute means "unset" + * (the PoolProperties default) rather than a NumberFormatException for + * the empty string, and a value that cannot be parsed - or does not fit + * the property - fails the configuration naming the property, instead of + * aborting the whole JNDI resource with an exception that only quotes + * the offending text. + */ + + @Test + void numericAttributesApplyIgnoringSurroundingWhitespace() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("maxActive", " 12 "); + attrs.setProperty("maxAge", " 86400000000 "); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertEquals(12, configured.getMaxActive(), + "\" 12 \" must be accepted, whitespace included"); + Assertions.assertEquals(86400000000L, configured.getMaxAge(), + "a long attribute must be accepted, whitespace included"); + } + + @Test + void emptyNumericAttributesLeaveThePropertiesUnset() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("maxActive", ""); + attrs.setProperty("maxWait", " "); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertEquals(PoolProperties.DEFAULT_MAX_ACTIVE, + configured.getMaxActive(), + "an empty numeric attribute must leave the default in place"); + Assertions.assertEquals(new PoolProperties().getMaxWait(), + configured.getMaxWait(), + "an all-whitespace numeric attribute must leave the default in place"); + } + + @Test + void unparseableNumericValueFailsTheParseNamingTheProperty() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("maxWait", "12 seconds"); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> DataSourceFactory.parsePoolProperties(attrs)); + Assertions.assertTrue(e.getMessage().contains("maxWait"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("12 seconds"), + "the failure must show the offending value, was: " + + e.getMessage()); + Assertions.assertInstanceOf(NumberFormatException.class, e.getCause(), + "the parse failure must be kept as the cause"); + } + + @Test + void numericValueBeyondThePropertyRangeFailsTheParseNamingTheProperty() { + java.util.Properties attrs = new java.util.Properties(); + // a millisecond value no int setter can hold (~2.7 years) + attrs.setProperty("minEvictableIdleTimeMillis", "86400000000"); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> DataSourceFactory.parsePoolProperties(attrs)); + Assertions.assertTrue(e.getMessage().contains("minEvictableIdleTimeMillis"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("86400000000"), + "the failure must show the offending value, was: " + + e.getMessage()); + } + + @Test + void blankStringAttributesLeaveThePropertiesUnset() { + // The same cleared-template policy the booleans, numerics, the + // isolation level and dataSourceJNDI follow. Pre-fix a stored empty + // defaultCatalog reached connection.setCatalog("") on every physical + // connection - driver-dependent from inert to a per-connect failure - + // and a blank dataSource aborted the resource with guidance about + // carrying the object, irrelevant to a value cleared to mean "off". + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultCatalog", ""); + attrs.setProperty("dataSource", " "); + attrs.setProperty("driverClassName", ""); + attrs.setProperty("jdbcInterceptors", " "); + attrs.setProperty("validationQuery", ""); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertNull(configured.getDefaultCatalog(), + "an empty defaultCatalog must leave the property unset"); + Assertions.assertNull(configured.getDataSource(), + "a blank dataSource must leave the property unset"); + Assertions.assertNull(configured.getDriverClassName(), + "a blank driverClassName must leave the property unset"); + Assertions.assertNull(configured.getJdbcInterceptors(), + "a blank jdbcInterceptors must leave the property unset"); + Assertions.assertNull(configured.getValidationQuery(), + "a blank validationQuery must leave the property unset," + + " routing validation to isValid()"); + } + + @Test + void blankDriverClassNameStillBuildsTheDriverManagerFallbackPool() + throws Exception { + // Pre-fix, the empty attribute defeated the connect path's documented + // DriverManager fallback (keyed on the property being null) and + // aborted the whole resource on loading the empty class. + 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")); + ref.add(new StringRefAddr("url", getUrl())); + ref.add(new StringRefAddr("username", "sa")); + ref.add(new StringRefAddr("password", "")); + ref.add(new StringRefAddr("driverClassName", "")); + ref.add(new StringRefAddr("jdbcInterceptors", " ")); + + DataSource ds = manage((DataSource) new DataSourceFactory() + .getObjectInstance(ref, new CompositeName("jdbc/BlankDS"), + null, null)); + + try (java.sql.Connection c = ds.getConnection(); + java.sql.ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the pool must build and connect through the fallback"); + } + } + + @Test + void driverClassNameAppliesIgnoringSurroundingWhitespace() { + // A Java class name carries no meaningful surrounding whitespace, so + // the padded shape a line-wrapped XML attribute yields must load the + // class it names, like the padded numeric and boolean spellings + // already do. Pre-fix only the blank test trimmed and the padded + // value went to the class loader verbatim. + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("driverClassName", " org.h2.Driver "); + + Assertions.assertEquals("org.h2.Driver", + DataSourceFactory.parsePoolProperties(attrs).getDriverClassName(), + "a padded class name must be applied trimmed"); + } + + @Test + void interceptorListWithRedundantSeparatorsStillBuildsThePool() + throws Exception { + // Pre-fix, one stray doubled separator - or one whitespace-only + // segment - made createPool() load the generated class "... + // pool.interceptor." and abort the whole resource with a + // SQLException whose message was null; the identical separator + // dropped at the end of the list was tolerated. + 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")); + ref.add(new StringRefAddr("url", getUrl())); + ref.add(new StringRefAddr("username", "sa")); + ref.add(new StringRefAddr("password", "")); + ref.add(new StringRefAddr("jdbcInterceptors", + " ; StatementFinalizer;;SlowQueryReport ; ")); + + DataSource ds = manage((DataSource) new DataSourceFactory() + .getObjectInstance(ref, new CompositeName("jdbc/SemiDS"), + null, null)); + + try (java.sql.Connection c = ds.getConnection(); + java.sql.ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the pool must build and borrow with its named chain"); + } + } + + @Test + void aConfiguredCatalogIsStoredUntrimmed() { + // The counterpart: a catalog name is a database identifier, so only + // the blank test trims - a configured name keeps its exact spelling. + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultCatalog", " My Catalog "); + + Assertions.assertEquals(" My Catalog ", + DataSourceFactory.parsePoolProperties(attrs).getDefaultCatalog(), + "a configured catalog name must be stored verbatim"); + } + + @Test + void blankObjectNameLeavesThePoolAtItsGeneratedName() { + // A cleared object_name attribute must not reach the pool as an + // empty quoted name - the JMX object name would carry an empty name + // component where the property being unset lets the pool name + // itself. + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("object_name", " "); + + String name = DataSourceFactory.parsePoolProperties(attrs).getName(); + + Assertions.assertNotEquals("\"\"", name, + "a blank object_name must not become an empty quoted name"); + Assertions.assertTrue(name.startsWith("Tomcat Connection Pool["), + "an unset object_name must leave the generated pool name," + + " was: " + name); + } + + @Test + void aConfiguredObjectNameIsQuotedButStoredVerbatim() { + // The counterpart, like the catalog: the name is quoted precisely so + // that "My Pool" survives the JMX object name, and only the blank + // test trims - a configured name keeps its exact spelling. + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("object_name", " My Pool "); + + Assertions.assertEquals("\" My Pool \"", + DataSourceFactory.parsePoolProperties(attrs).getName(), + "a configured pool name must be quoted but stored verbatim"); + } + /* A factory-built pool, H2-backed, with one extra attribute. */ private DataSource factoryPoolWithAutoCommit(String value) throws Exception { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java index 27de71ba15..99548b1808 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java @@ -172,26 +172,46 @@ class TestJdbcInterceptorConfigParsing { } @Test - void extraSemicolonsProduceAnExtraDefinitionOnlyInSomePositions() { - // This one DOES get an extra/empty definition + void redundantSemicolonsAreCosmeticInEveryPosition() { + // A redundant separator must mean the same thing wherever it + // appears: split() always dropped the trailing one, the property + // parse always tolerated one inside the parentheses, and an empty + // leading or doubled one used to survive as a definition without a + // class name - which the pool could only try to load as + // "...pool.interceptor.", failing the whole start with a message + // that named nothing. All positions now yield the same chain. + String[][] spellings = { + { ";LeadingSemicolonInterceptor", "LeadingSemicolonInterceptor" }, + { "Doubled;;SemicolonInterceptor", "Doubled,SemicolonInterceptor" }, + { "TrailingSemicolonInterceptor;", "TrailingSemicolonInterceptor" }, + { "TrailingWhitespaceInterceptor; ", "TrailingWhitespaceInterceptor" }, + }; + for (String[] spelling : spellings) { + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(spelling[0]); + InterceptorDefinition[] jiDefs = props.getJdbcInterceptorsAsArray(); + Assertions.assertNotNull(jiDefs); + String[] names = spelling[1].split(","); + Assertions.assertEquals(names.length + 1, jiDefs.length, + spelling[0] + " must parse as trap plus the named interceptors"); + Assertions.assertEquals(TrapException.class.getName(), jiDefs[0].getClassName()); + for (int i = 0; i < names.length; i++) { + Assertions.assertEquals(names[i], jiDefs[i + 1].getClassName(), + spelling[0] + " must keep its real interceptors, in order"); + } + } + } + + @Test + void aSegmentWithoutANameContributesNoDefinition() { + // "(", " ( " and a segment that opens straight on the property list + // name no interceptor at all; keeping such a definition only moved + // the failure from the parser to the pool start PoolProperties props = new PoolProperties(); - props.setJdbcInterceptors(";EmptyLeadingSemiInterceptor"); + props.setJdbcInterceptors("(parm=value);NamedInterceptor"); InterceptorDefinition[] jiDefs = props.getJdbcInterceptorsAsArray(); - Assertions.assertNotNull(jiDefs); - Assertions.assertEquals(3, jiDefs.length); - - // This one does NOT get an extra/empty definition (no trailing whitespace) - props = new PoolProperties(); - props.setJdbcInterceptors("EmptyTrailingSemiInterceptor;"); - jiDefs = props.getJdbcInterceptorsAsArray(); - Assertions.assertNotNull(jiDefs); Assertions.assertEquals(2, jiDefs.length); - - // This one DOES get an extra/empty definition (with trailing whitespace) - props = new PoolProperties(); - props.setJdbcInterceptors("EmptyTrailingSemiInterceptor; "); - jiDefs = props.getJdbcInterceptorsAsArray(); - Assertions.assertNotNull(jiDefs); - Assertions.assertEquals(3, jiDefs.length); + Assertions.assertEquals(TrapException.class.getName(), jiDefs[0].getClassName()); + Assertions.assertEquals("NamedInterceptor", jiDefs[1].getClassName()); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java index 689a8a73ed..57ee3de17a 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java @@ -213,6 +213,65 @@ class TestPoolProperties { "a credential injected outside the string must survive reconfiguration"); } + @Test + void credentialsOverridingTheStringSurviveAReconfigureThatDropsThem() { + // The factory's own shape: the string names user/password, and the + // username/password attributes are injected over the string's pairs. + // Pre-fix the injection overwrote the values but the string stayed the + // owner of the keys, so a JMX reconfigure whose string no longer named + // the credentials removed them from the driver properties while the + // credential fields still held them. + Properties attrs = new Properties(); + attrs.setProperty("connectionProperties", + "keep=yes;user=fromstring;password=fromstring"); + attrs.setProperty("username", DEFAULT_USER); + attrs.setProperty("password", DEFAULT_PASSWD); + + PoolProperties configured = + (PoolProperties) DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertEquals(DEFAULT_USER, + configured.getDbProperties().getProperty("user"), + "the configured username must override the string's pair"); + Assertions.assertEquals(DEFAULT_PASSWD, + configured.getDbProperties().getProperty("password"), + "the configured password must override the string's pair"); + + // the JMX reconfigure path + configured.setConnectionProperties("keep=again"); + + Assertions.assertEquals("again", + configured.getDbProperties().getProperty("keep"), + "a pair the new string names must carry the new value"); + Assertions.assertEquals(DEFAULT_USER, + configured.getDbProperties().getProperty("user"), + "a credential the fields still carry must not be stripped by a" + + " reconfigure that dropped the key from the string"); + Assertions.assertEquals(DEFAULT_PASSWD, + configured.getDbProperties().getProperty("password"), + "a credential the fields still carry must not be stripped by a" + + " reconfigure that dropped the key from the string"); + } + + @Test + void aCredentialEntryOnlyTheStringCarriesStaysTheStringsOwn() { + // Counterpart pin: with the field unset the entry is the string's, so + // the reconfigure really does drop it - the re-assert must not turn + // setConnectionProperties into a credential keeper that outlives both + // the field and the string. + PoolProperties properties = new PoolProperties(); + properties.setConnectionProperties("user=fromstring"); + + Assertions.assertEquals("fromstring", + properties.getDbProperties().getProperty("user"), + "the string's pair must be applied while no username is set"); + + properties.setConnectionProperties("keep=yes"); + + Assertions.assertNull(properties.getDbProperties().getProperty("user"), + "the string must keep owning the entry while the field is unset"); + } + @Test void clearedConnectionPropertiesRemoveEveryParsedPair() { PoolProperties properties = new PoolProperties(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
