Implement more and louder validation of config properties.

Author: swaldman

CHANGELOG

@@ -1,3 +1,5 @@
+	-- Implement more and "louder" (log ugly stack traces) validation of
+	   config properties.
 	-- Modify former InUseLock (now InternalUseLock) and NewProxyConnection
 	   to use ReentrantLock rather than native monitors, to prevent pinning
 	   when clients run on loom virtual threads.

bean/beangen/com/mchange/v2/c3p0/impl/WrapperConnectionPoolDataSourceBase.beangen-xml

@@ -3,6 +3,7 @@
   <imports>
      <specific>javax.sql.DataSource</specific>
      <specific>com.mchange.v2.c3p0.cfg.C3P0Config</specific>
+     <specific>com.mchange.v2.c3p0.cfg.Validators</specific>
   </imports>
   <modifiers>
      <modifier>public</modifier>
@@ -149,7 +150,7 @@
        <type>String</type>
        <name>contextClassLoaderSource</name>
        <constrained/>
-       <default-value>C3P0Config.initializeStringPropertyVar("contextClassLoaderSource", C3P0Defaults.contextClassLoaderSource())</default-value>
+       <default-value>C3P0Config.initializeStringPropertyVar("contextClassLoaderSource", C3P0Defaults.contextClassLoaderSource(), Validators.ContextClassLoaderSource)</default-value>
        <getter><modifiers><modifier>public</modifier><modifier>synchronized</modifier></modifiers></getter>
        <setter><modifiers><modifier>public</modifier><modifier>synchronized</modifier></modifiers></setter>
     </property>
@@ -177,7 +178,7 @@
     <property>
        <type>String</type>
        <name>markSessionBoundaries</name>
-       <default-value>C3P0Config.initializeStringPropertyVar("markSessionBoundaries", C3P0Defaults.markSessionBoundaries())</default-value>
+       <default-value>C3P0Config.initializeStringPropertyVar("markSessionBoundaries", C3P0Defaults.markSessionBoundaries(), Validators.MarkSessionBoundaries)</default-value>
        <constrained/>
        <getter><modifiers><modifier>public</modifier><modifier>synchronized</modifier></modifiers></getter>
        <setter><modifiers><modifier>public</modifier><modifier>synchronized</modifier></modifiers></setter>

dev-notes.md

@@ -1,16 +1,23 @@
 # dev-notes
 
 * It looks like throwing of PropertyVetoException may not be implemented for ostensibly constrained parameters:
-  - `connectionTesterClassName` (should classname params be constrained?)
+  - `connectionTesterClassName` (should classname params be constrained? no actual implementation I think)
   - `contextClassLoaderSource`
   - `markSessionBoundaries`
-  - `taskRunnerFactoryClassName` (should classname params be constrained?)
+  - `taskRunnerFactoryClassName` (should classname params be constrained? no actual implementation I think)
   - `userOverridesAsString`
   It might be disruptive to "fix" this now, as installations may have configs that are stable despite
   bad (vetoable) settings.
+  
+  PropertyVetoException appears to be thrown with explicit calls to setters, but not on implementation via config.
+  Variables are initialized to config values, circumventing validation.
+  
+  Okay. We've added a way to validate String config properties, and validation for `contextClassLoaderSource` and `markSessionBoundaries`.
 
   If class name properties should be constrained, probably so too ought be
   - `connectionCustomizerClassName`
+  but making it so would be an incompatible API change at this point, callers of the setter would have to prepare
+  for `ProppertyVetoException`, which now they do not.
 
 * When testing against intentionally chaotic configs, we occasionally see, at debug level FINE:
   ```plaintext

src/com/mchange/v2/c3p0/cfg/C3P0Config.java

@@ -518,13 +518,27 @@ public static String initializeUserOverridesAsString()
     public static Map initializeExtensions()
     { return getExtensions( null ); }
 
-    public static String initializeStringPropertyVar(String propKey, String dflt)
+    public static String initializeStringPropertyVar(String propKey, String dflt, Validator<String> validator)
     {
-	String out = getUnspecifiedUserProperty( propKey, null );
-	if (out == null) out = dflt;
-	return out;
+        String out = getUnspecifiedUserProperty( propKey, null );
+        try
+            {
+                if (out == null) out = dflt;
+                else if (validator != null) out = validator.validate(out);
+                return out;
+            }
+        catch (InvalidConfigException ice)
+            {
+                logger.log(MLevel.WARNING, "'" + out + "' is not a legal value for property '" + propKey +
+                           "'. Using default value: " + dflt, ice);
+                return dflt;
+            }
     }
 
+    public static String initializeStringPropertyVar(String propKey, String dflt)
+    { return initializeStringPropertyVar(propKey, dflt, null); }
+
+
     public static int initializeIntPropertyVar(String propKey, int dflt)
     {
 	boolean set = false;
@@ -535,13 +549,13 @@ public static int initializeIntPropertyVar(String propKey, int dflt)
 	    {
 		try 
 		    { 
-			out = Integer.parseInt( outStr.trim() ); 
+			out = Integer.parseInt( outStr.trim() );
 			set = true;
 		    }
 		catch (NumberFormatException e)
 		    {
-			logger.info("'" + outStr + "' is not a legal value for property '" + propKey +
-				    "'. Using default value: " + dflt);
+			logger.log(MLevel.WARNING, "'" + outStr + "' is not a legal value for property '" + propKey +
+				   "'. Using default value: " + dflt, e);
 		    }
 	    }
 
@@ -567,8 +581,8 @@ public static boolean initializeBooleanPropertyVar(String propKey, boolean dflt)
 		    }
 		catch (IllegalArgumentException e)
 		    {
-			logger.info("'" + outStr + "' is not a legal value for property '" + propKey +
-				    "'. Using default value: " + dflt);
+			logger.log(MLevel.WARNING, "'" + outStr + "' is not a legal value for property '" + propKey +
+				                   "'. Using default value: " + dflt, e);
 		    }
 	    }
 

src/com/mchange/v2/c3p0/cfg/InvalidConfigException.java

@@ -0,0 +1,10 @@
+package com.mchange.v2.c3p0.cfg;
+
+public final class InvalidConfigException extends Exception
+{
+    public InvalidConfigException( String message, Throwable cause )
+    { super( message, cause ); }
+
+    public InvalidConfigException( String message )
+    { this( message, null ); }
+}

src/com/mchange/v2/c3p0/cfg/Validator.java

@@ -0,0 +1,6 @@
+package com.mchange.v2.c3p0.cfg;
+
+interface Validator<T>
+{
+    public T validate( T value ) throws InvalidConfigException;
+}

src/com/mchange/v2/c3p0/cfg/Validators.java

@@ -0,0 +1,51 @@
+package com.mchange.v2.c3p0.cfg;
+
+public final class Validators
+{
+    static abstract class AbstractValidator<T> implements Validator<T>
+    {
+        String param;
+
+        AbstractValidator( String param )
+        { this.param = param; }
+
+        abstract T _validate( T value ) throws InvalidConfigException;
+
+        public T validate( T value ) throws InvalidConfigException
+        {
+            try
+                { return _validate(value); }
+            catch (InvalidConfigException ice)
+                { throw ice; }
+            catch (Exception e)
+                { throw new InvalidConfigException("While validating '" + param + "', encountered Exception: " + e, e); }
+        }
+    }
+
+    public static Validator<String> MarkSessionBoundaries = new AbstractValidator<String>("markSessionBoundaries")
+    {
+        String _validate( String value ) throws InvalidConfigException
+        {
+            String out = value.toLowerCase();
+            if ("always".equals(out)||"never".equals(out)||"if-no-statement-cache".equals(out))
+                return out;
+            else
+                throw new InvalidConfigException(param + " must be one of 'always', 'never', or 'if-no-statement-cache'. Found '" + value + "'.");
+        }
+    };
+
+    public static Validator<String> ContextClassLoaderSource = new AbstractValidator<String>("contextClassLoaderSource")
+    {
+        String _validate( String value ) throws InvalidConfigException
+        {
+             String out = value.toLowerCase();
+             if ("caller".equals(out)||"library".equals(out)||"none".equals(out))
+                 return out;
+             else
+                 throw new InvalidConfigException(param + " must be one of 'caller', 'library', or 'none'. Found '" + value + "'.");
+        }
+    };
+
+    private Validators()
+    {}
+}