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 01df2b0a9154ae10568c85648a6bab31e3df77ee Author: opencode <[email protected]> AuthorDate: Sun Oct 25 17:18:39 2026 +0100 Report naming factory property-set failures at warn level with accurate messages GenericNamingResourcesFactory swallowed every kind of property-set failure at debug level (FINE, suppressed by the default logging configuration): a value that did not parse for the matched setter, a setter that threw, a SecurityException or an IllegalAccessException, and a property with no usable setter. In all of these cases the half-configured object was still returned, bound into JNDI and, for a DataSource singleton, registered with JMX, while the operator saw noth- ing at all. The one message that was emitted, from an inverted empty-if at the call site, also claimed 'No setter found' even when a setter had been found and the value or the setter had failed, which is actively misleading when debug logging is enabled to diagnose a problem. Report the failures at warn level, with accurate wording, both in the catch blocks of setProperty() and at the call site in getObjectInstance(). This matches the maintained org.apache.tomcat.util.IntrospectionUtils, which logs the same failures at warn and leaves reporting of conversion failures to the caller. The propagation of VirtualMachineError thrown by a setter is unchanged, as that is the behavior of the repo-wide ExceptionUtils.handleThrowable convention, which tomcat-jdbc.jar cannot use directly. A correctly configured Resource produces no new warnings: the reference metadata addresses that a ResourceRef always carries are already filtered (commit 82d3d0161e), so only genuinely ignored configuration attributes now warn. Add a regression test that captures the log records produced while applying a reference with a bad numeric value, an unknown property and a throwing setter, and asserts each failure is reported at warn level or above. --- .../jdbc/naming/GenericNamingResourcesFactory.java | 35 +++++----- .../tomcat/jdbc/naming/TestNamingFactory.java | 80 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/naming/GenericNamingResourcesFactory.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/naming/GenericNamingResourcesFactory.java index b019904612..b265c2a8c6 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/naming/GenericNamingResourcesFactory.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/naming/GenericNamingResourcesFactory.java @@ -98,10 +98,10 @@ public class GenericNamingResourcesFactory implements ObjectFactory { if (addr.getContent()!=null) { value = addr.getContent().toString(); } - if (setProperty(o, param, value)) { - - } else { - log.debug("Property not configured[" + param + "]. No setter found on[" + type + "]."); + if (!setProperty(o, param, value)) { + log.warn("Property [" + param + "] was not applied to [" + + type + "]: no usable setter found, or the " + + "configured value could not be applied."); } } return o; @@ -230,26 +230,27 @@ public class GenericNamingResourcesFactory implements ObjectFactory { } } catch (IllegalArgumentException ex2) { - log.warn("IAE " + o + " " + name + " " + value, ex2); + log.warn("Unable to set property [" + name + "] with value [" + + value + "] on [" + o.getClass().getName() + "]", ex2); } catch (SecurityException ex1) { - if (log.isDebugEnabled()) { - log.debug("IntrospectionUtils: SecurityException for " + - o.getClass() + " " + name + "=" + value + ")", ex1); - } + log.warn("SecurityException setting property [" + name + + "] with value [" + value + "] on [" + + o.getClass().getName() + "]", ex1); } catch (IllegalAccessException iae) { - if (log.isDebugEnabled()) { - log.debug("IntrospectionUtils: IllegalAccessException for " + - o.getClass() + " " + name + "=" + value + ")", iae); - } + log.warn("IllegalAccessException setting property [" + name + + "] with value [" + value + "] on [" + + o.getClass().getName() + "]", iae); } catch (InvocationTargetException ie) { + // In the spirit of ExceptionUtils.handleThrowable(), which is not + // available to tomcat-jdbc.jar: propagate the fatal + // VirtualMachineError from the setter and report everything else. Throwable cause = ie.getCause(); if (cause instanceof VirtualMachineError) { throw (VirtualMachineError) cause; } - if (log.isDebugEnabled()) { - log.debug("IntrospectionUtils: InvocationTargetException for " + - o.getClass() + " " + name + "=" + value + ")", ie); - } + log.warn("Setter for property [" + name + "] with value [" + + value + "] threw an exception on [" + + o.getClass().getName() + "]", ie); } return false; } 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 e01ed2b79a..860eabb00f 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 @@ -16,6 +16,13 @@ */ package org.apache.tomcat.jdbc.naming; +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + import javax.naming.CompositeName; import javax.naming.Name; import javax.naming.Reference; @@ -226,6 +233,79 @@ class TestNamingFactory extends JdbcPoolBaseTest { } } + @Test + void propertySetFailuresAreReportedAsWarnings() throws Exception { + Logger logger = Logger.getLogger( + GenericNamingResourcesFactory.class.getName()); + Level originalLevel = logger.getLevel(); + final List<LogRecord> records = new ArrayList<>(); + Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + records.add(record); + } + @Override + public void flush() { + // no buffering + } + @Override + public void close() { + // nothing to release + } + }; + logger.addHandler(handler); + logger.setLevel(Level.FINEST); + try { + Reference ref = new Reference(WarnTarget.class.getName(), + null, null); + // "count" matches setCount(int) but the value does not parse + ref.add(new StringRefAddr("count", "12o")); + // no setter exists at all + ref.add(new StringRefAddr("noSuchProperty", "x")); + // the matched setter throws + ref.add(new StringRefAddr("boom", "go")); + + Object result = new GenericNamingResourcesFactory() + .getObjectInstance( + ref, new CompositeName("test/warn"), null, null); + assertInstanceOf(WarnTarget.class, result); + // the half-configured object is still returned, as documented + assertEquals(-1, ((WarnTarget) result).getCount()); + } finally { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + for (String property : new String[] { "count", "noSuchProperty", + "boom" }) { + boolean warned = false; + for (LogRecord record : records) { + if (record.getLevel().intValue() >= Level.WARNING.intValue() + && record.getMessage() != null + && record.getMessage().contains("[" + property + "]")) { + warned = true; + break; + } + } + assertTrue(warned, "failure to apply property [" + property + + "] must be reported at warn level or above"); + } + } + + /** Target exercising the property-set failure paths. */ + public static class WarnTarget { + private int count = -1; + + public int getCount() { + return count; + } + public void setCount(int count) { + this.count = count; + } + public void setBoom(String value) { + throw new IllegalStateException("setter always fails"); + } + } + /** Plain configuration target for the generic naming factory. */ public static class NamingTarget { private String name; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
