This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit d6236405622e45240899b5bc31fc1d74fe56b5ae Author: opencode <[email protected]> AuthorDate: Sun Nov 1 14:17:45 2026 +0100 Fail hard on unapplied properties in GenericNamingResourcesFactory A JNDI lookup that could not apply a configured property silently returned a half-built bean, which the container then cached for singleton resources. Now every address that is not reference metadata either reaches the object or fails the lookup with a NamingException naming what was not applied. - Booleans accept only true or false, ignoring case and surrounding whitespace - not the BeanFactory anything-but-true-is-false coercion - and an address carrying no content is applied to nothing. - Apply the BeanFactory-parity conversions: int, long, short, byte, float, double, char and boolean in primitive and boxed form, plus InetAddress, with the empty-value traps closed (no fabricated first character, no loopback for an empty host). - Consult a setProperty(name, value) bag only when the bean has no same-named single-argument setter; a boxed Boolean bag return is evaluated like a primitive one; the bag never absorbs a value the bean's own setter could not be given. - Failure logs and messages no longer echo configured values: a failing password setter would otherwise write the secret to the log. Fatal VirtualMachineErrors from constructors and setters propagate; the container's ExceptionUtils cannot be referenced from tomcat-jdbc.jar and the check is inlined until the helper is shared. - The class javadoc states the full lookup contract: any Reference is accepted, the loader order, one new public-no-arg-constructed instance per lookup, the reserved metadata addresses, the supported types and the strict boolean grammar. Direct regression tests cover every fix. --- .../jdbc/naming/GenericNamingResourcesFactory.java | 266 +++++++++--- .../tomcat/jdbc/pool/util/LocalStrings.properties | 9 +- .../tomcat/jdbc/naming/TestNamingFactory.java | 470 +++++++++++++++++++-- 3 files changed, 659 insertions(+), 86 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 ec1dde2d3a..3b39793a22 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 @@ -20,11 +20,13 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; import java.util.Hashtable; +import java.util.List; import java.util.Set; import javax.naming.Context; @@ -53,14 +55,42 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * port="1527"/> * </code> * </pre> + * <p> + * The factory accepts any {@link Reference}, not only the container-managed + * {@code ResourceRef} that a {@code <Resource>} element produces. The + * referenced class is loaded with the factory's own class loader first and + * the thread context class loader second, and a new instance built by its + * public no-argument constructor is created for every lookup. Reference + * metadata the container attaches ({@code factory}, {@code singleton}, + * {@code scope}, {@code auth}, {@code description} and {@code forceString}) + * is skipped; every other address is treated as a bean property. + * <p> + * A property is applied through a {@code setFoo(String)} setter when one + * exists, otherwise through a single-argument setter whose parameter type the + * configured value can be built from: {@code int}, {@code long}, + * {@code short}, {@code byte}, {@code float}, {@code double}, {@code char} + * and {@code boolean} - each in its primitive and boxed form - and + * {@code java.net.InetAddress}. Booleans accept only {@code true} or + * {@code false}, ignoring case and surrounding whitespace - not the + * anything-but-{@code true}-is-{@code false} coercion of + * {@code org.apache.naming.factory.BeanFactory} - and an address carrying no + * content is applied to nothing: missing content is not a configured value. + * When no {@code setFoo} setter exists at all, a two-argument + * {@code setProperty(name, value)} property bag is consulted; when one + * exists but its type cannot be built from the configured value, the bag is + * not consulted - what the bean's own setter rejects must not be silently + * absorbed there. + * <p> + * A property that cannot be applied - no usable setter, a value the setter + * type cannot be built from, or a setter that throws - fails the lookup with + * a {@code NamingException}, so a partially configured resource never + * reaches a caller and, in particular, is never cached by the container for + * a singleton resource. */ public class GenericNamingResourcesFactory implements ObjectFactory { - /** - * Creates a new GenericNamingResourcesFactory instance. - */ - public GenericNamingResourcesFactory() { - // Default constructor - } + + /** Default constructor. */ + public GenericNamingResourcesFactory() { } private static final Log log = LogFactory.getLog(GenericNamingResourcesFactory.class); @@ -120,6 +150,7 @@ public class GenericNamingResourcesFactory implements ObjectFactory { throw ne; } + List<String> notApplied = new ArrayList<>(); while (refs.hasMoreElements()) { RefAddr addr = refs.nextElement(); String param = addr.getType(); @@ -131,17 +162,51 @@ public class GenericNamingResourcesFactory implements ObjectFactory { if (addr.getContent() != null) { value = addr.getContent().toString(); } - if (!setProperty(o, param, value)) { - log.warn(MessageFactory.get("genericNamingResourcesFactory.propertyNotApplied", param, type)); + SetResult set = setProperty(o, param, value); + if (set != SetResult.APPLIED) { + notApplied.add(param); + if (set == SetResult.UNREPORTED) { + // The property simply could not be converted or no usable + // setter was found; a matched setter that failed has + // already been reported by setProperty with its cause. + log.warn(MessageFactory.get("genericNamingResourcesFactory.propertyNotApplied", param, type)); + } } } + // A half-configured object must not be handed to the caller: for a + // singleton resource the container caches the returned object, so + // every later lookup would silently get the misconfigured bean. Fail + // the lookup instead. + if (!notApplied.isEmpty()) { + NamingException ne = new NamingException(MessageFactory.get( + "genericNamingResourcesFactory.propertiesNotApplied", notApplied, type, name)); + throw ne; + } return o; } - @SuppressWarnings("null") // setPropertyMethodVoid can't be null when used - private static boolean setProperty(Object o, String name, String value) { + /** + * Outcome of an attempt to apply one configured property. + */ + private enum SetResult { + /** The value reached the object. */ + APPLIED, + /** The property could not be applied and the cause was reported. */ + REPORTED, + /** The property could not be applied; the caller reports it. */ + UNREPORTED + } + + private static SetResult setProperty(Object o, String name, String value) { if (log.isTraceEnabled()) { - log.trace("IntrospectionUtils: setProperty(" + o.getClass() + " " + name + "=" + value + ")"); + log.trace("setProperty(" + o.getClass() + " " + name + "=" + value + ")"); + } + + // Missing content is not a configured value: no setter - String or + // typed - and no property bag is invoked with a fabricated null + // that would wipe the configured default. + if (value == null) { + return SetResult.UNREPORTED; } String setter = "set" + capitalize(name); @@ -158,16 +223,23 @@ public class GenericNamingResourcesFactory implements ObjectFactory { "java.lang.String".equals(paramT[0].getName())) { methods[i].invoke(o, new Object[] { value }); - return true; + return SetResult.APPLIED; } } - // Try a setFoo ( int ) or ( boolean ) + // No String setter: try a setFoo whose single parameter type the + // value can be built from (int, long, short, byte, float, double, + // char, boolean - primitive or boxed - or InetAddress). A setter + // that matched but could not be fed is recorded, so that the + // property bag below knows the bean has its own opinion about + // this property. + boolean typedSetterMatched = false; for (int i = 0; i < methods.length; i++) { boolean ok = true; if (setter.equals(methods[i].getName()) && methods[i].getParameterTypes().length == 1) { // Match - find the type and invoke it + typedSetterMatched = true; Class<?> paramType = methods[i].getParameterTypes()[0]; Object[] params = new Object[1]; @@ -185,34 +257,84 @@ public class GenericNamingResourcesFactory implements ObjectFactory { } catch (NumberFormatException ex) { ok = false; } + // Try a setFoo ( short ) + } else if ("java.lang.Short".equals(paramType.getName()) || + "short".equals(paramType.getName())) { + try { + params[0] = Short.valueOf(value); + } catch (NumberFormatException ex) { + ok = false; + } + // Try a setFoo ( byte ) + } else if ("java.lang.Byte".equals(paramType.getName()) || + "byte".equals(paramType.getName())) { + try { + params[0] = Byte.valueOf(value); + } catch (NumberFormatException ex) { + ok = false; + } + // Try a setFoo ( float ) + } else if ("java.lang.Float".equals(paramType.getName()) || + "float".equals(paramType.getName())) { + try { + params[0] = Float.valueOf(value); + } catch (NumberFormatException ex) { + ok = false; + } + // Try a setFoo ( double ) + } else if ("java.lang.Double".equals(paramType.getName()) || + "double".equals(paramType.getName())) { + try { + params[0] = Double.valueOf(value); + } catch (NumberFormatException ex) { + ok = false; + } + // Try a setFoo ( char or Character ) + } else if ("java.lang.Character".equals(paramType.getName()) || + "char".equals(paramType.getName())) { + // Take the first character of the value, like + // BeanFactory. Empty content has no first + // character: do not fabricate one. + if (value.isEmpty()) { + ok = false; + } else { + params[0] = Character.valueOf(value.charAt(0)); + } // Try a setFoo ( boolean ) } else if ("java.lang.Boolean".equals(paramType.getName()) || "boolean".equals(paramType.getName())) { // Boolean.valueOf() reports everything it does not - // recognise, missing content included, as false. - // Missing content is not a configured value: let the - // caller report the property as not applied. - if (value == null || value.isEmpty()) { - ok = false; + // recognise, empty content included, as false. + // Anything that does not spell a boolean - empty + // content, whitespace-padded or misspelled values - + // is not a configured value: let the caller report + // the property as not applied instead of silently + // applying false. Same policy as the pool's own + // DataSourceFactory.parseBoolean(). + String trimmed = value.trim(); + if ("true".equalsIgnoreCase(trimmed)) { + params[0] = Boolean.TRUE; + } else if ("false".equalsIgnoreCase(trimmed)) { + params[0] = Boolean.FALSE; } else { - params[0] = Boolean.valueOf(value); + ok = false; } // Try a setFoo ( InetAddress ) } else if ("java.net.InetAddress".equals(paramType.getName())) { - // getByName() documents null and empty content as the - // loopback address instead of failing, and a missing + // getByName() documents the empty string as the + // loopback address instead of failing, and an empty // host is not a request for loopback: do not invoke // the setter with a fabricated address. - if (value == null || value.isEmpty()) { + if (value.isEmpty()) { ok = false; } else { try { params[0] = InetAddress.getByName(value); } catch (UnknownHostException exc) { if (log.isDebugEnabled()) { - log.debug("IntrospectionUtils: Unable to resolve host name:" + value); + log.debug("Unable to resolve host name: " + value); } ok = false; } @@ -220,18 +342,19 @@ public class GenericNamingResourcesFactory implements ObjectFactory { // Unknown type } else { - // Unsupported parameter type. Do not invoke the - // method with a null value. Let the caller fall back - // to setProperty(name, value) or handle the failure. + // Unsupported parameter type: the value cannot be + // built for it, so the setter is not invoked with a + // fabricated null and the property is reported as not + // applied. ok = false; if (log.isDebugEnabled()) { - log.debug("IntrospectionUtils: Unknown type " + paramType.getName()); + log.debug("Unknown type " + paramType.getName()); } } if (ok) { methods[i].invoke(o, params); - return true; + return SetResult.APPLIED; } } @@ -245,7 +368,12 @@ public class GenericNamingResourcesFactory implements ObjectFactory { Class<?>[] paramT = methods[i].getParameterTypes(); if (paramT.length == 2 && paramT[0].isAssignableFrom(String.class) && paramT[1].isAssignableFrom(String.class)) { - if (methods[i].getReturnType() == Boolean.TYPE) { + // A boxed Boolean bag refuses a property the same + // way a primitive boolean one does: it belongs in + // the bucket whose return value is evaluated, not + // in the fire-and-forget bucket. + if (methods[i].getReturnType() == Boolean.TYPE + || methods[i].getReturnType() == Boolean.class) { setPropertyMethodBool = methods[i]; } else { setPropertyMethodVoid = methods[i]; @@ -255,44 +383,36 @@ public class GenericNamingResourcesFactory implements ObjectFactory { } // Ok, no setXXX found, try a setProperty("name", "value") + // A setXXX matched but no supported overload converted the value: + // consulting the bag here would silently absorb the value the + // bean's own setter rejected and report success, leaving the real + // bean property at its default - a partially configured resource. if (setPropertyMethodBool != null || setPropertyMethodVoid != null) { - Object[] params = new Object[2]; - params[0] = name; - params[1] = value; - if (setPropertyMethodBool != null) { - try { - return ((Boolean) setPropertyMethodBool.invoke(o, params)).booleanValue(); - } catch (IllegalArgumentException biae) { - // Defensive: the recorded method's parameter types - // were checked above, so invoking it with the name - // and value should not fail on the arguments; if it - // somehow does, fall back to the other method. - if (setPropertyMethodVoid != null) { - setPropertyMethodVoid.invoke(o, params); - return true; - } else { - throw biae; - } - } - } else { - setPropertyMethodVoid.invoke(o, params); - return true; + if (typedSetterMatched) { + return SetResult.UNREPORTED; } + return applyPropertyBag(o, name, value, + setPropertyMethodBool, setPropertyMethodVoid); } } catch (IllegalArgumentException ex2) { log.warn(MessageFactory.get("genericNamingResourcesFactory.unableToSetProperty", - name, value, o.getClass().getName()), ex2); + name, o.getClass().getName()), ex2); + return SetResult.REPORTED; } catch (SecurityException ex1) { log.warn(MessageFactory.get("genericNamingResourcesFactory.securityExceptionSettingProperty", - name, value, o.getClass().getName()), ex1); + name, o.getClass().getName()), ex1); + return SetResult.REPORTED; } catch (IllegalAccessException iae) { log.warn(MessageFactory.get("genericNamingResourcesFactory.illegalAccessExceptionSettingProperty", - name, value, o.getClass().getName()), iae); + name, o.getClass().getName()), iae); + return SetResult.REPORTED; } 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. + // The configured value is deliberately not echoed: a failing + // password setter would otherwise write the secret to the log. Throwable cause = ie.getCause(); if (cause instanceof ThreadDeath) { throw (ThreadDeath) cause; @@ -301,9 +421,45 @@ public class GenericNamingResourcesFactory implements ObjectFactory { throw (VirtualMachineError) cause; } log.warn(MessageFactory.get("genericNamingResourcesFactory.setterThrew", - name, value, o.getClass().getName()), ie); + name, o.getClass().getName()), ie); + return SetResult.REPORTED; + } + return SetResult.UNREPORTED; + } + + /** + * Apply a configured property through the setProperty(name, value) + * fallback. Called from inside setProperty's try, so every exception the + * bags can raise - including the failure of the defensive fallback + * below - reaches the caller's typed handlers and is reported as a + * failed property, instead of escaping unconverted from within a catch + * block the outer handlers cannot guard. + */ + @SuppressWarnings("null") // voidBag can't be null where it is used + private static SetResult applyPropertyBag(Object o, String name, + String value, Method boolBag, Method voidBag) + throws IllegalAccessException, InvocationTargetException { + Object[] params = new Object[2]; + params[0] = name; + params[1] = value; + if (boolBag != null) { + try { + return Boolean.TRUE.equals(boolBag.invoke(o, params)) + ? SetResult.APPLIED : SetResult.UNREPORTED; + } catch (IllegalArgumentException biae) { + // Defensive: the recorded method's parameter types were + // checked when it was saved, so invoking it with the name + // and value should not fail on the arguments; if it somehow + // does, fall back to the other method. + if (voidBag == null) { + throw biae; + } + } } - return false; + // Reached with a usable void bag, or after the boolean bag refused + // to run on the very arguments it was recorded for. + voidBag.invoke(o, params); + return SetResult.APPLIED; } /** 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 766d9628e8..c83f0a47f1 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 @@ -117,13 +117,14 @@ fairBlockingQueue.unsupportedToArrayT=<T> T[] toArray(T[] a) genericNamingResourcesFactory.classNotFound=Class [{0}] of reference [{1}] was not found genericNamingResourcesFactory.constructorThrew=The constructor of class [{0}] of reference [{1}] threw an exception and the object cannot be created -genericNamingResourcesFactory.illegalAccessExceptionSettingProperty=IllegalAccessException setting property [{0}] with value [{1}] on [{2}] +genericNamingResourcesFactory.illegalAccessExceptionSettingProperty=IllegalAccessException setting property [{0}] on [{1}] genericNamingResourcesFactory.noResourceType=No resource type (the "type" attribute) is defined for reference [{0}]; GenericNamingResourcesFactory needs the class of the object to create. genericNamingResourcesFactory.noUsableConstructor=Class [{0}] of reference [{1}] has no usable public no-argument constructor and cannot be created by this factory +genericNamingResourcesFactory.propertiesNotApplied=The object of class [{1}] bound as [{2}] was not created: the configured properties [{0}] could not be applied. genericNamingResourcesFactory.propertyNotApplied=Property [{0}] was not applied to [{1}]: no usable setter found, or the configured value could not be applied. -genericNamingResourcesFactory.securityExceptionSettingProperty=SecurityException setting property [{0}] with value [{1}] on [{2}] -genericNamingResourcesFactory.setterThrew=Setter for property [{0}] with value [{1}] threw an exception on [{2}] -genericNamingResourcesFactory.unableToSetProperty=Unable to set property [{0}] with value [{1}] on [{2}] +genericNamingResourcesFactory.securityExceptionSettingProperty=SecurityException setting property [{0}] on [{1}] +genericNamingResourcesFactory.setterThrew=Setter for property [{0}] threw an exception on [{1}] +genericNamingResourcesFactory.unableToSetProperty=Unable to set property [{0}] on [{1}] jmxConnectionPool.droppingNotification=Dropping notification of type [{0}] for pool [{1}]: the notification delivery queue ({2} deliveries) is full 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 9d091013a2..57abf46aee 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 @@ -61,6 +61,19 @@ public class TestNamingFactory extends JdbcPoolBaseTest { return ref; } + /* + * A property that cannot be applied fails the lookup: the partially + * configured object must never be returned, for a singleton resource + * the container caches whatever object the factory returns. Targets + * that a test must inspect despite the failure self-register in the + * constructor, which runs before any property is applied. + */ + private void assertLookupFails(Reference ref, String boundName) { + Assertions.assertThrows(NamingException.class, + () -> new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName(boundName), null, null)); + } + @Test void referenceProducesWorkingConfiguredPool() throws Exception { Name name = new CompositeName("jdbc/FunctionalDS"); @@ -243,6 +256,108 @@ public class TestNamingFactory extends JdbcPoolBaseTest { Assertions.assertTrue(target.isEnabled()); } + @Test + void namingFactoryAppliesTheReferenceImplementationTypes() + throws Exception { + // BeanFactory converts all of these; the generic factory must not + // drop properties that the reference implementation configures + Reference ref = new Reference(WideTarget.class.getName(), null, null); + ref.add(new StringRefAddr("small", "7")); + ref.add(new StringRefAddr("tiny", "-3")); + ref.add(new StringRefAddr("ratio", "1.5")); + ref.add(new StringRefAddr("precise", "0.25")); + ref.add(new StringRefAddr("initial", "configured")); + ref.add(new StringRefAddr("boxed", "x")); + + Object result = new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName("test/wide"), null, null); + Assertions.assertInstanceOf(WideTarget.class, result); + WideTarget target = (WideTarget) result; + Assertions.assertEquals((short) 7, target.getSmall()); + Assertions.assertEquals((byte) -3, target.getTiny()); + Assertions.assertEquals(1.5f, target.getRatio(), 0.0f); + Assertions.assertEquals(0.25d, target.getPrecise(), 0.0d); + Assertions.assertEquals('c', target.getInitial()); + Assertions.assertEquals(Character.valueOf('x'), target.getBoxed()); + } + + @Test + void outOfRangeNumberFailsTheLookupNamingTheProperty() throws Exception { + Reference ref = new Reference(WideTarget.class.getName(), null, null); + // no short can hold this: the conversion must fail and, like every + // other unapplied property, fail the lookup + ref.add(new StringRefAddr("small", "99999")); + + NamingException e = Assertions.assertThrows(NamingException.class, + () -> new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName("test/widebad"), null, null)); + Assertions.assertTrue(e.getMessage().contains("small"), + "the failure must name the property, was: " + e.getMessage()); + } + + @Test + void missingContentIsNotAppliedToAStringSetter() throws Exception { + Reference ref = new Reference(NamingTarget.class.getName(), null, null); + // pre-fix the String setter received a fabricated null, wiping the + // configured default and reporting success; missing content is not + // a value to apply, to a String setter no less than to a typed one + NamingTarget.last = null; + ref.add(new StringRefAddr("name", null)); + + assertLookupFails(ref, "test/nullstring"); + NamingTarget target = NamingTarget.last; + Assertions.assertNotNull(target); + Assertions.assertEquals("untitled", target.getName(), + "the default must survive an address with no content"); + } + + /** Target with the numeric and character setters BeanFactory supports. */ + public static class WideTarget { + private short small; + private byte tiny; + private float ratio; + private double precise; + private char initial; + private Character boxed; + + public short getSmall() { + return small; + } + public void setSmall(short small) { + this.small = small; + } + public byte getTiny() { + return tiny; + } + public void setTiny(byte tiny) { + this.tiny = tiny; + } + public float getRatio() { + return ratio; + } + public void setRatio(float ratio) { + this.ratio = ratio; + } + public double getPrecise() { + return precise; + } + public void setPrecise(double precise) { + this.precise = precise; + } + public char getInitial() { + return initial; + } + public void setInitial(char initial) { + this.initial = initial; + } + public Character getBoxed() { + return boxed; + } + public void setBoxed(Character boxed) { + this.boxed = boxed; + } + } + /* * An address that carries no content is a value the factory cannot * convert, not a value to fabricate: Boolean.valueOf(null or "") is @@ -289,12 +404,11 @@ public class TestNamingFactory extends JdbcPoolBaseTest { ref.add(new StringRefAddr("addr", null)); ref.add(new StringRefAddr("host", "")); - Object result = new GenericNamingResourcesFactory() - .getObjectInstance( - ref, new CompositeName("test/converted"), - null, null); - Assertions.assertInstanceOf(ConvertedTarget.class, result); - ConvertedTarget target = (ConvertedTarget) result; + ConvertedTarget.last = null; + assertLookupFails(ref, "test/converted"); + ConvertedTarget target = ConvertedTarget.last; + Assertions.assertNotNull(target, + "the object is created before properties are applied"); for (String property : new String[] { "enabled", "checked", "flag", "marker", "addr", "host" }) { Assertions.assertFalse(target.isInvoked(property), @@ -364,8 +478,97 @@ public class TestNamingFactory extends JdbcPoolBaseTest { target.getHost()); } + /* + * Boolean.valueOf() maps everything that is not exactly "true" (ignore + * case) to false: whitespace-padded and misspelled configuration values + * were silently applied as false and reported as a successful set. The + * boolean conversion must accept only content that spells a boolean, + * like the pool's own DataSourceFactory.parseBoolean() does; anything + * else must leave the setter uninvoked and be reported as not applied. + */ + + @Test + void unrecognizedBooleanContentIsNotAppliedAsFalse() 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(ConvertedTarget.class.getName(), + null, null); + // the realistic configuration mistakes: a value padded by an XML + // line wrap, a synonym and a typo. Pre-fix every one of these + // was applied as false with no diagnostic. + ref.add(new StringRefAddr("checked", "Yes")); + ref.add(new StringRefAddr("flag", "1")); + ref.add(new StringRefAddr("marker", "ture")); + // surrounding whitespace around genuine content is still + // accepted, like DataSourceFactory.parseBoolean() does + ref.add(new StringRefAddr("enabled", " True ")); + + ConvertedTarget.last = null; + // checked/flag/marker are not applicable, so the lookup fails; + // enabled=" True " is applied first, verified via the target's + // self-registration before the failure + assertLookupFails(ref, "test/badboolean"); + ConvertedTarget target = ConvertedTarget.last; + Assertions.assertNotNull(target); + Assertions.assertTrue(target.isEnabled(), + "\" True \" is a padded true and must be applied"); + for (String property : new String[] { "checked", "flag", + "marker" }) { + Assertions.assertFalse(target.isInvoked(property), + "the setter for [" + property + + "] must not be invoked with content that" + + " does not spell a boolean"); + } + Assertions.assertFalse(target.isChecked()); + Assertions.assertNull(target.getFlag(), + "the default must survive unrecognized content"); + Assertions.assertNull(target.getMarker(), + "the default must survive unrecognized content"); + } finally { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + for (String property : new String[] { "checked", "flag", + "marker" }) { + boolean warned = false; + for (LogRecord record : records) { + if (record.getLevel().intValue() >= Level.WARNING.intValue() + && record.getMessage() != null + && record.getMessage().contains("[" + property + "]")) { + warned = true; + break; + } + } + Assertions.assertTrue(warned, "failure to apply property [" + + property + + "] must be reported at warn level or above"); + } + } + /** Target with primitive and boxed boolean and InetAddress setters. */ public static class ConvertedTarget { + // set by the constructor so a test can inspect the created object + // even when the factory fails the lookup before returning it + static ConvertedTarget last; private final java.util.Set<String> invoked = new java.util.HashSet<>(); private boolean enabled; @@ -375,6 +578,10 @@ public class TestNamingFactory extends JdbcPoolBaseTest { private java.net.InetAddress addr; private java.net.InetAddress host; + public ConvertedTarget() { + last = this; + } + public boolean isInvoked(String name) { return invoked.contains(name); } @@ -431,10 +638,10 @@ public class TestNamingFactory extends JdbcPoolBaseTest { // null argument, wiping the configured default. ref.add(new StringRefAddr("props", "anything")); - Object result = new GenericNamingResourcesFactory().getObjectInstance( - ref, new CompositeName("test/unsupported"), null, null); - Assertions.assertInstanceOf(UnsupportedTypeTarget.class, result); - UnsupportedTypeTarget target = (UnsupportedTypeTarget) result; + UnsupportedTypeTarget.last = null; + assertLookupFails(ref, "test/unsupported"); + UnsupportedTypeTarget target = UnsupportedTypeTarget.last; + Assertions.assertNotNull(target); Assertions.assertFalse(target.isPropsInvoked(), "setter with an unsupported parameter type must not be " + "invoked with a null value"); @@ -452,10 +659,10 @@ public class TestNamingFactory extends JdbcPoolBaseTest { // must not be invoked with null instead. ref.add(new StringRefAddr("port", "12o")); - Object result = new GenericNamingResourcesFactory().getObjectInstance( - ref, new CompositeName("test/overloaded"), null, null); - Assertions.assertInstanceOf(OverloadedTarget.class, result); - OverloadedTarget target = (OverloadedTarget) result; + OverloadedTarget.last = null; + assertLookupFails(ref, "test/overloaded"); + OverloadedTarget target = OverloadedTarget.last; + Assertions.assertNotNull(target); Assertions.assertFalse(target.isPropsInvoked(), "the unsupported overload must not be invoked with null when " + "the numeric conversion fails"); @@ -465,11 +672,16 @@ public class TestNamingFactory extends JdbcPoolBaseTest { /** Target with a setter of a type the factory cannot convert to. */ public static class UnsupportedTypeTarget { + static UnsupportedTypeTarget last; static final java.util.Properties DEFAULT_PROPS = new java.util.Properties(); private java.util.Properties props = DEFAULT_PROPS; private boolean propsInvoked; + public UnsupportedTypeTarget() { + last = this; + } + public java.util.Properties getProps() { return props; } @@ -484,10 +696,15 @@ public class TestNamingFactory extends JdbcPoolBaseTest { /** Target with an overloaded setter, one supported and one not. */ public static class OverloadedTarget { + static OverloadedTarget last; private int port = -1; private java.util.Properties props; private boolean propsInvoked; + public OverloadedTarget() { + last = this; + } + public int getPort() { return port; } @@ -576,12 +793,10 @@ public class TestNamingFactory extends JdbcPoolBaseTest { // one can accept those arguments ref.add(new StringRefAddr("whatever", "v")); - Object result = new GenericNamingResourcesFactory() - .getObjectInstance( - ref, new CompositeName("test/decoy"), null, null); - Assertions.assertInstanceOf(DecoySetPropertyTarget.class, result); + DecoySetPropertyTarget.last = null; + assertLookupFails(ref, "test/decoy"); Assertions.assertFalse( - ((DecoySetPropertyTarget) result).isInvoked(), + DecoySetPropertyTarget.last.isInvoked(), "a setProperty overload the factory cannot call with the " + "configured name and value must not run"); } finally { @@ -613,8 +828,13 @@ public class TestNamingFactory extends JdbcPoolBaseTest { * invoked with two arguments, fails reflectively. */ public static class DecoySetPropertyTarget { + static DecoySetPropertyTarget last; private boolean invoked; + public DecoySetPropertyTarget() { + last = this; + } + public boolean isInvoked() { return invoked; } @@ -702,7 +922,113 @@ public class TestNamingFactory extends JdbcPoolBaseTest { } @Test - void propertySetFailuresAreReportedAsWarnings() throws Exception { + void boxedBooleanBagRefusalFailsTheLookup() throws Exception { + Reference ref = new Reference(RefusingBoxedBagTarget.class.getName(), + null, null); + // pre-fix the boxed java.lang.Boolean return landed in the + // fire-and-forget bucket: the bag's refusal was counted as applied + // and the unconfigured object was returned + ref.add(new StringRefAddr("anything", "v")); + + NamingException e = Assertions.assertThrows(NamingException.class, + () -> new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName("test/boxedrefuse"), + null, null)); + Assertions.assertTrue(e.getMessage().contains("anything"), + "the failure must name the refused property, was: " + + e.getMessage()); + } + + @Test + void boxedBooleanBagAcceptanceApplies() throws Exception { + Reference ref = new Reference(AcceptingBoxedBagTarget.class.getName(), + null, null); + ref.add(new StringRefAddr("databaseName", "sample")); + + Object result = new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName("test/boxedaccept"), null, null); + Assertions.assertInstanceOf(AcceptingBoxedBagTarget.class, result); + Assertions.assertEquals(java.util.Collections.singletonMap( + "databaseName", "sample"), + ((AcceptingBoxedBagTarget) result).getBag(), + "a boxed Boolean bag that returns TRUE must apply like " + + "the primitive boolean one"); + } + + /** Property bag whose boxed Boolean return refuses every property. */ + public static class RefusingBoxedBagTarget { + public Boolean setProperty(@SuppressWarnings("unused") String name, + @SuppressWarnings("unused") String value) { + return Boolean.FALSE; + } + } + + /** Property bag whose boxed Boolean return accepts every property. */ + public static class AcceptingBoxedBagTarget { + private final java.util.Map<String, String> bag = + new java.util.LinkedHashMap<>(); + + public Boolean setProperty(String name, String value) { + bag.put(name, value); + return Boolean.TRUE; + } + public java.util.Map<String, String> getBag() { + return bag; + } + } + + @Test + void conversionFailureIsNotAbsorbedByThePropertyBag() throws Exception { + Reference ref = new Reference( + TypedSetterAndBagTarget.class.getName(), null, null); + // setCount(int) matches the property but the value does not build an + // int; pre-fix the permissive bag was still consulted, so the typo + // landed in the bag, the bean property kept its default and the + // lookup reported success + TypedSetterAndBagTarget.last = null; + ref.add(new StringRefAddr("count", "12o")); + + NamingException e = Assertions.assertThrows(NamingException.class, + () -> new GenericNamingResourcesFactory().getObjectInstance( + ref, new CompositeName("test/mixedbag"), + null, null)); + Assertions.assertTrue(e.getMessage().contains("count"), + "the failure must name the property, was: " + e.getMessage()); + TypedSetterAndBagTarget target = TypedSetterAndBagTarget.last; + Assertions.assertNotNull(target); + Assertions.assertEquals(-1, target.getCount()); + Assertions.assertTrue(target.getBag().isEmpty(), + "the bag must not absorb a value the typed setter rejected, " + + "bag was: " + target.getBag()); + } + + /** Target with a typed setter and a permissive property bag. */ + public static class TypedSetterAndBagTarget { + static TypedSetterAndBagTarget last; + private int count = -1; + private final java.util.Map<String, String> bag = + new java.util.LinkedHashMap<>(); + + public TypedSetterAndBagTarget() { + last = this; + } + + public void setProperty(String name, String value) { + bag.put(name, value); + } + public void setCount(int count) { + this.count = count; + } + public int getCount() { + return count; + } + public java.util.Map<String, String> getBag() { + return bag; + } + } + + @Test + void unappliedPropertiesFailTheLookupAndAreReportedAsWarnings() throws Exception { Logger logger = Logger.getLogger( GenericNamingResourcesFactory.class.getName()); Level originalLevel = logger.getLevel(); @@ -733,12 +1059,25 @@ public class TestNamingFactory extends JdbcPoolBaseTest { // the matched setter throws ref.add(new StringRefAddr("boom", "go")); - Object result = new GenericNamingResourcesFactory() - .getObjectInstance( - ref, new CompositeName("test/warn"), null, null); - Assertions.assertInstanceOf(WarnTarget.class, result); - // the half-configured object is still returned, as documented - Assertions.assertEquals(-1, ((WarnTarget) result).getCount()); + // a property that cannot be applied fails the lookup: the + // half-configured object must not be returned, for a singleton + // resource the container would cache it for every later lookup + NamingException e = Assertions.assertThrows( + NamingException.class, + () -> new GenericNamingResourcesFactory() + .getObjectInstance( + ref, new CompositeName("test/warn"), + null, null)); + for (String property : new String[] { "count", "noSuchProperty", + "boom" }) { + Assertions.assertTrue(e.getMessage().contains(property), + "the failure must name the unapplied property [" + + property + "], was: " + e.getMessage()); + } + Assertions.assertTrue( + e.getMessage().contains(WarnTarget.class.getName()), + "the failure must name the resource class, was: " + + e.getMessage()); } finally { logger.removeHandler(handler); logger.setLevel(originalLevel); @@ -759,6 +1098,75 @@ public class TestNamingFactory extends JdbcPoolBaseTest { } } + /* + * A failing setter must not write the configured value to the log: the + * property may well be a password, and the warn level is on by default. + * The real cause is reported once, by setProperty; the caller must not + * add a second, misleading "no usable setter found" warning on top. + */ + + @Test + void failureLogsDoNotEchoConfiguredValues() 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.WARNING); + final String secret = "S3cr3t-Pa55word"; + NamingException thrown; + try { + Reference ref = new Reference(WarnTarget.class.getName(), + null, null); + // the matched setter throws with the secret in hand: pre-fix the + // WARN line echoed the value, and the caller added a second WARN + // claiming no usable setter had been found + ref.add(new StringRefAddr("password", secret)); + + thrown = Assertions.assertThrows(NamingException.class, + () -> new GenericNamingResourcesFactory() + .getObjectInstance( + ref, new CompositeName("test/secret"), + null, null)); + } finally { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + Assertions.assertFalse(thrown.getMessage().contains(secret), + "the failure must not echo the configured value, was: " + + thrown.getMessage()); + int reported = 0; + for (LogRecord record : records) { + Assertions.assertFalse( + record.getMessage() != null + && record.getMessage().contains(secret), + "a failing setter must not write the configured value to" + + " the log: " + record.getMessage()); + if (record.getLevel().intValue() >= Level.WARNING.intValue() + && record.getMessage() != null + && record.getMessage().contains("[password]")) { + reported++; + } + } + Assertions.assertEquals(1, reported, + "the failing setter must be reported once, at warn level, " + + "without a duplicate claim of a missing setter"); + } + /** Target exercising the property-set failure paths. */ public static class WarnTarget { private int count = -1; @@ -772,6 +1180,9 @@ public class TestNamingFactory extends JdbcPoolBaseTest { public void setBoom(@SuppressWarnings("unused") String value) { throw new IllegalStateException("setter always fails"); } + public void setPassword(@SuppressWarnings("unused") String password) { + throw new IllegalStateException("password setter always fails"); + } } @Test @@ -833,11 +1244,16 @@ public class TestNamingFactory extends JdbcPoolBaseTest { /** Plain configuration target for the generic naming factory. */ public static class NamingTarget { - private String name; + static NamingTarget last; + private String name = "untitled"; private int count; private long ratio; private boolean enabled; + public NamingTarget() { + last = this; + } + public String getName() { return name; } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
