ConfigBeanFactory: validate against bean's implied schema

The advantage of this is that rather than throwing a single
"ConfigException.Missing" we can give the user a list of all
missing and wrong-typed settings for the entire bean up front.

Validation is not perfect (just as the regular checkValid method
isn't, it doesn't find all problems that actually getting the
value could find), but it does find the common cases of leaving
out a setting or having a blatantly incorrect type.

config/src/main/java/com/typesafe/config/ConfigBeanFactory.java

@@ -32,6 +32,8 @@ public class ConfigBeanFactory {
      * @return an instance of the class populated with data from the config
      * @throws ConfigException.BadBean
      *     If something is wrong with the JavaBean
+     * @throws ConfigException.ValidationFailed
+     *     If the config doesn't conform to the bean's implied schema
      * @throws ConfigException
      *     Can throw the same exceptions as the getters on <code>Config</code>
      */

config/src/main/java/com/typesafe/config/impl/ConfigBeanImpl.java

@@ -6,7 +6,9 @@ import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.time.Duration;
@@ -14,6 +16,8 @@ import java.time.Duration;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigException;
 import com.typesafe.config.ConfigMemorySize;
+import com.typesafe.config.ConfigValue;
+import com.typesafe.config.ConfigValueType;
 
 public class ConfigBeanImpl {
 
@@ -22,12 +26,16 @@ public class ConfigBeanImpl {
      * may change.
      */
     public static <T> T createInternal(Config config, Class<T> clazz) {
-        Map<String, ?> configAsMap = config.root().unwrapped();
-        Map<String, Object> configProps = new HashMap<String, Object>();
-        Map<String,String> originalNames = new HashMap<String, String>();
-        for (Map.Entry<String, ?> configProp : configAsMap.entrySet()) {
-            configProps.put(ConfigImplUtil.toCamelCase(configProp.getKey()), configProp.getValue());
-            originalNames.put(ConfigImplUtil.toCamelCase(configProp.getKey()),configProp.getKey());
+        if (((SimpleConfig)config).root().resolveStatus() != ResolveStatus.RESOLVED)
+            throw new ConfigException.NotResolved(
+                    "need to Config#resolve() a config before using it to initialize a bean, see the API docs for Config#resolve()");
+
+        Map<String, AbstractConfigValue> configProps = new HashMap<String, AbstractConfigValue>();
+        Map<String, String> originalNames = new HashMap<String, String>();
+        for (Map.Entry<String, ConfigValue> configProp : config.root().entrySet()) {
+            String camelName = ConfigImplUtil.toCamelCase(configProp.getKey());
+            configProps.put(camelName, (AbstractConfigValue) configProp.getValue());
+            originalNames.put(camelName, configProp.getKey());
         }
 
         BeanInfo beanInfo = null;
@@ -38,24 +46,55 @@ public class ConfigBeanImpl {
         }
 
         try {
-            T bean = clazz.newInstance();
+            List<PropertyDescriptor> beanProps = new ArrayList<PropertyDescriptor>();
             for (PropertyDescriptor beanProp : beanInfo.getPropertyDescriptors()) {
                 if (beanProp.getReadMethod() == null || beanProp.getWriteMethod() == null) {
                     continue;
                 }
+                beanProps.add(beanProp);
+            }
+
+            // Try to throw all validation issues at once (this does not comprehensively
+            // find every issue, but it should find common ones).
+            List<ConfigException.ValidationProblem> problems = new ArrayList<ConfigException.ValidationProblem>();
+            for (PropertyDescriptor beanProp : beanProps) {
                 Method setter = beanProp.getWriteMethod();
-                Object configValue = configProps.get(beanProp.getName());
+                Class parameterClass = setter.getParameterTypes()[0];
+                ConfigValueType expectedType = getValueTypeOrNull(parameterClass);
+                if (expectedType != null) {
+                    String name = originalNames.get(beanProp.getName());
+                    if (name == null)
+                        name = beanProp.getName();
+                    Path path = Path.newKey(name);
+                    AbstractConfigValue configValue = configProps.get(beanProp.getName());
+                    if (configValue != null) {
+                        SimpleConfig.checkValid(path, expectedType, configValue, problems);
+                    } else {
+                        SimpleConfig.addMissing(problems, expectedType, path, config.origin());
+                    }
+                }
+            }
+
+            if (!problems.isEmpty()) {
+                throw new ConfigException.ValidationFailed(problems);
+            }
+
+            // Fill in the bean instance
+            T bean = clazz.newInstance();
+            for (PropertyDescriptor beanProp : beanProps) {
+                Method setter = beanProp.getWriteMethod();
+                ConfigValue configValue = configProps.get(beanProp.getName());
+                Object unwrapped;
                 if (configValue == null) {
                     throw new ConfigException.Missing(beanProp.getName());
                 }
-                if (configValue instanceof Map) {
-                    configValue = createInternal(config.getConfig(originalNames.get(beanProp.getDisplayName())), beanProp.getPropertyType());
+                if (configValue instanceof SimpleConfigObject) {
+                    unwrapped = createInternal(config.getConfig(originalNames.get(beanProp.getDisplayName())), beanProp.getPropertyType());
                 } else {
                     Class parameterClass = setter.getParameterTypes()[0];
-                    configValue = getValueWithAutoConversion(parameterClass, config, originalNames.get(beanProp.getDisplayName()));
+                    unwrapped = getValueWithAutoConversion(parameterClass, config, originalNames.get(beanProp.getDisplayName()));
                 }
-                setter.invoke(bean, configValue);
-
+                setter.invoke(bean, unwrapped);
             }
             return bean;
         } catch (InstantiationException e) {
@@ -90,4 +129,33 @@ public class ConfigBeanImpl {
 
         return config.getAnyRef(configPropName);
     }
+
+    // null if we can't easily say
+    private static ConfigValueType getValueTypeOrNull(Class<?> parameterClass) {
+        if (parameterClass == Boolean.class || parameterClass == boolean.class) {
+            return ConfigValueType.BOOLEAN;
+        } else if (parameterClass == Byte.class || parameterClass == byte.class) {
+            return ConfigValueType.NUMBER;
+        } else if (parameterClass == Short.class || parameterClass == short.class) {
+            return ConfigValueType.NUMBER;
+        } else if (parameterClass == Integer.class || parameterClass == int.class) {
+            return ConfigValueType.NUMBER;
+        } else if (parameterClass == Double.class || parameterClass == double.class) {
+            return ConfigValueType.NUMBER;
+        } else if (parameterClass == Long.class || parameterClass == long.class) {
+            return ConfigValueType.NUMBER;
+        } else if (parameterClass == String.class) {
+            return ConfigValueType.STRING;
+        } else if (parameterClass == Duration.class) {
+            return null;
+        } else if (parameterClass == ConfigMemorySize.class) {
+            return null;
+        } else if (parameterClass.isAssignableFrom(List.class)) {
+            return ConfigValueType.LIST;
+        } else if (parameterClass.isAssignableFrom(Map.class)) {
+            return ConfigValueType.OBJECT;
+        } else {
+            return null;
+        }
+    }
 }

config/src/main/java/com/typesafe/config/impl/SimpleConfig.java

@@ -726,31 +726,54 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
         accumulator.add(new ConfigException.ValidationProblem(path.render(), origin, problem));
     }
 
+    private static String getDesc(ConfigValueType type) {
+        return type.name().toLowerCase();
+    }
+
     private static String getDesc(ConfigValue refValue) {
         if (refValue instanceof AbstractConfigObject) {
             AbstractConfigObject obj = (AbstractConfigObject) refValue;
-            if (obj.isEmpty())
-                return "object";
-            else
+            if (!obj.isEmpty())
                 return "object with keys " + obj.keySet();
-        } else if (refValue instanceof SimpleConfigList) {
-            return "list";
+            else
+                return getDesc(refValue.valueType());
         } else {
-            return refValue.valueType().name().toLowerCase();
+            return getDesc(refValue.valueType());
         }
     }
 
     private static void addMissing(List<ConfigException.ValidationProblem> accumulator,
-            ConfigValue refValue, Path path, ConfigOrigin origin) {
+                                   String refDesc, Path path, ConfigOrigin origin) {
         addProblem(accumulator, path, origin, "No setting at '" + path.render() + "', expecting: "
-                + getDesc(refValue));
+                   + refDesc);
+    }
+
+    private static void addMissing(List<ConfigException.ValidationProblem> accumulator,
+            ConfigValue refValue, Path path, ConfigOrigin origin) {
+        addMissing(accumulator, getDesc(refValue), path, origin);
+    }
+
+    // JavaBean stuff uses this
+    static void addMissing(List<ConfigException.ValidationProblem> accumulator,
+            ConfigValueType refType, Path path, ConfigOrigin origin) {
+        addMissing(accumulator, getDesc(refType), path, origin);
+    }
+
+    private static void addWrongType(List<ConfigException.ValidationProblem> accumulator,
+            String refDesc, AbstractConfigValue actual, Path path) {
+        addProblem(accumulator, path, actual.origin(), "Wrong value type at '" + path.render()
+                   + "', expecting: " + refDesc + " but got: "
+                   + getDesc(actual));
     }
 
     private static void addWrongType(List<ConfigException.ValidationProblem> accumulator,
             ConfigValue refValue, AbstractConfigValue actual, Path path) {
-        addProblem(accumulator, path, actual.origin(), "Wrong value type at '" + path.render()
-                + "', expecting: " + getDesc(refValue) + " but got: "
-                        + getDesc(actual));
+        addWrongType(accumulator, getDesc(refValue), actual, path);
+    }
+
+    private static void addWrongType(List<ConfigException.ValidationProblem> accumulator,
+            ConfigValueType refType, AbstractConfigValue actual, Path path) {
+        addWrongType(accumulator, getDesc(refType), actual, path);
     }
 
     private static boolean couldBeNull(AbstractConfigValue v) {
@@ -759,23 +782,32 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
     }
 
     private static boolean haveCompatibleTypes(ConfigValue reference, AbstractConfigValue value) {
-        if (couldBeNull((AbstractConfigValue) reference) || couldBeNull(value)) {
+        if (couldBeNull((AbstractConfigValue) reference)) {
             // we allow any setting to be null
             return true;
-        } else if (reference instanceof AbstractConfigObject) {
+        } else {
+            return haveCompatibleTypes(reference.valueType(), value);
+        }
+    }
+
+    private static boolean haveCompatibleTypes(ConfigValueType referenceType, AbstractConfigValue value) {
+        if (referenceType == ConfigValueType.NULL || couldBeNull(value)) {
+            // we allow any setting to be null
+            return true;
+        } else if (referenceType == ConfigValueType.OBJECT) {
             if (value instanceof AbstractConfigObject) {
                 return true;
             } else {
                 return false;
             }
-        } else if (reference instanceof SimpleConfigList) {
+        } else if (referenceType == ConfigValueType.LIST) {
             // objects may be convertible to lists if they have numeric keys
             if (value instanceof SimpleConfigList || value instanceof SimpleConfigObject) {
                 return true;
             } else {
                 return false;
             }
-        } else if (reference instanceof ConfigString) {
+        } else if (referenceType == ConfigValueType.STRING) {
             // assume a string could be gotten as any non-collection type;
             // allows things like getMilliseconds including domain-specific
             // interpretations of strings
@@ -784,7 +816,7 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
             // assume a string could be gotten as any non-collection type
             return true;
         } else {
-            if (reference.valueType() == value.valueType()) {
+            if (referenceType == value.valueType()) {
                 return true;
             } else {
                 return false;
@@ -833,6 +865,22 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
         }
     }
 
+    // Used by the JavaBean-based validator
+    static void checkValid(Path path, ConfigValueType referenceType, AbstractConfigValue value,
+            List<ConfigException.ValidationProblem> accumulator) {
+        if (haveCompatibleTypes(referenceType, value)) {
+            if (referenceType == ConfigValueType.LIST && value instanceof SimpleConfigObject) {
+                // attempt conversion of indexed object to list
+                AbstractConfigValue listValue = DefaultTransformer.transform(value,
+                        ConfigValueType.LIST);
+                if (!(listValue instanceof SimpleConfigList))
+                    addWrongType(accumulator, referenceType, value, path);
+            }
+        } else {
+            addWrongType(accumulator, referenceType, value, path);
+        }
+    }
+
     private static void checkValid(Path path, ConfigValue reference, AbstractConfigValue value,
             List<ConfigException.ValidationProblem> accumulator) {
         // Unmergeable is supposed to be impossible to encounter in here

config/src/test/java/beanconfig/NoFoundPropBeanConfig.java

@@ -1,14 +0,0 @@
-package beanconfig;
-
-public class NoFoundPropBeanConfig extends TestBeanConfig{
-
-    private String propNotListedInConfig;
-
-    public String getPropNotListedInConfig() {
-        return propNotListedInConfig;
-    }
-
-    public void setPropNotListedInConfig(String propNotListedInConfig) {
-        this.propNotListedInConfig = propNotListedInConfig;
-    }
-}

config/src/test/java/beanconfig/ValidationBeanConfig.java

@@ -0,0 +1,44 @@
+package beanconfig;
+
+import java.util.List;
+
+public class ValidationBeanConfig extends TestBeanConfig{
+
+    private String propNotListedInConfig;
+    private int shouldBeInt;
+    private boolean shouldBeBoolean;
+    private List<Integer> shouldBeList;
+
+    public String getPropNotListedInConfig() {
+        return propNotListedInConfig;
+    }
+
+    public void setPropNotListedInConfig(String propNotListedInConfig) {
+        this.propNotListedInConfig = propNotListedInConfig;
+    }
+
+    public int getShouldBeInt() {
+        return shouldBeInt;
+    }
+
+    public void setShouldBeInt(int v) {
+        shouldBeInt = v;
+    }
+
+    public boolean getShouldBeBoolean() {
+        return shouldBeBoolean;
+    }
+
+    public void setShouldBeBoolean(boolean v) {
+        shouldBeBoolean = v;
+    }
+
+    public List<Integer> getShouldBeList() {
+        return shouldBeList;
+    }
+
+    public void setShouldBeList(List<Integer> v) {
+        shouldBeList = v;
+    }
+
+}

config/src/test/resources/beanconfig/beanconfig01.conf

@@ -58,5 +58,9 @@
         "secondAsNumber" : 1000,
         "halfSecond" : 0.5s
     },
-
+    "validation" : {
+      "shouldBeInt" : true,
+      "should-be-boolean" : 42,
+      "should-be-list" : "hello"
+    }
 }

config/src/test/scala/com/typesafe/config/impl/ConfigBeanFactoryTest.scala

@@ -38,15 +38,20 @@ class ConfigBeanFactoryTest extends TestUtils {
     }
 
     @Test
-    def testUnknownProp() {
+    def testValidation() {
         val configIs: InputStream = this.getClass().getClassLoader().getResourceAsStream("beanconfig/beanconfig01.conf")
         val config: Config = ConfigFactory.parseReader(new InputStreamReader(configIs),
-            ConfigParseOptions.defaults.setSyntax(ConfigSyntax.CONF)).resolve
-        val expected = intercept[ConfigException.Missing] {
-            ConfigBeanFactory.create(config, classOf[NoFoundPropBeanConfig])
+            ConfigParseOptions.defaults.setSyntax(ConfigSyntax.CONF)).resolve.getConfig("validation")
+        val e = intercept[ConfigException.ValidationFailed] {
+            ConfigBeanFactory.create(config, classOf[ValidationBeanConfig])
         }
-        // TODO this error message should have the config name not the camelcase name
-        assertTrue(expected.getMessage.contains("propNotListedInConfig"))
+
+        val expecteds = Seq(Missing("propNotListedInConfig", 61, "string"),
+            WrongType("shouldBeInt", 62, "number", "boolean"),
+            WrongType("should-be-boolean", 63, "boolean", "number"),
+            WrongType("should-be-list", 64, "list", "string"))
+
+        checkValidationException(e, expecteds)
     }
 
     @Test

config/src/test/scala/com/typesafe/config/impl/TestUtils.scala

@@ -25,6 +25,7 @@ import java.util.concurrent.Callable
 import com.typesafe.config._
 import scala.reflect.ClassTag
 import scala.reflect.classTag
+import scala.collection.JavaConverters._
 import language.implicitConversions
 
 abstract trait TestUtils {
@@ -740,4 +741,53 @@ abstract trait TestUtils {
 
     protected def quoteJsonString(s: String): String =
         ConfigImplUtil.renderJsonString(s)
+
+    sealed abstract class Problem(path: String, line: Int) {
+        def check(p: ConfigException.ValidationProblem) {
+            assertEquals("matching path", path, p.path())
+            assertEquals("matching line for " + path, line, p.origin().lineNumber())
+        }
+
+        protected def assertMessage(p: ConfigException.ValidationProblem, re: String) {
+            assertTrue("didn't get expected message for " + path + ": got '" + p.problem() + "'",
+                p.problem().matches(re))
+        }
+    }
+
+    case class Missing(path: String, line: Int, expected: String) extends Problem(path, line) {
+        override def check(p: ConfigException.ValidationProblem) {
+            super.check(p)
+            val re = "No setting.*" + path + ".*expecting.*" + expected + ".*"
+            assertMessage(p, re)
+        }
+    }
+
+    case class WrongType(path: String, line: Int, expected: String, got: String) extends Problem(path, line) {
+        override def check(p: ConfigException.ValidationProblem) {
+            super.check(p)
+            val re = "Wrong value type.*" + path + ".*expecting.*" + expected + ".*got.*" + got + ".*"
+            assertMessage(p, re)
+        }
+    }
+
+    case class WrongElementType(path: String, line: Int, expected: String, got: String) extends Problem(path, line) {
+        override def check(p: ConfigException.ValidationProblem) {
+            super.check(p)
+            val re = "List at.*" + path + ".*wrong value type.*expecting.*" + expected + ".*got.*element of.*" + got + ".*"
+            assertMessage(p, re)
+        }
+    }
+
+    protected def checkValidationException(e: ConfigException.ValidationFailed, expecteds: Seq[Problem]) {
+        val problems = e.problems().asScala.toIndexedSeq.sortBy(_.path).sortBy(_.origin.lineNumber)
+
+        //for (problem <- problems)
+        //    System.err.println(problem.origin().description() + ": " + problem.path() + ": " + problem.problem())
+
+        for ((problem, expected) <- problems zip expecteds) {
+            expected.check(problem)
+        }
+        assertEquals("found expected validation problems, got '" + problems + "' and expected '" + expecteds + "'",
+            expecteds.size, problems.size)
+    }
 }

config/src/test/scala/com/typesafe/config/impl/ValidationTest.scala

@@ -13,55 +13,6 @@ import scala.io.Source
 
 class ValidationTest extends TestUtils {
 
-    sealed abstract class Problem(path: String, line: Int) {
-        def check(p: ConfigException.ValidationProblem) {
-            assertEquals("matching path", path, p.path())
-            assertEquals("matching line", line, p.origin().lineNumber())
-        }
-
-        protected def assertMessage(p: ConfigException.ValidationProblem, re: String) {
-            assertTrue("didn't get expected message for " + path + ": got '" + p.problem() + "'",
-                p.problem().matches(re))
-        }
-    }
-
-    case class Missing(path: String, line: Int, expected: String) extends Problem(path, line) {
-        override def check(p: ConfigException.ValidationProblem) {
-            super.check(p)
-            val re = "No setting.*" + path + ".*expecting.*" + expected + ".*"
-            assertMessage(p, re)
-        }
-    }
-
-    case class WrongType(path: String, line: Int, expected: String, got: String) extends Problem(path, line) {
-        override def check(p: ConfigException.ValidationProblem) {
-            super.check(p)
-            val re = "Wrong value type.*" + path + ".*expecting.*" + expected + ".*got.*" + got + ".*"
-            assertMessage(p, re)
-        }
-    }
-
-    case class WrongElementType(path: String, line: Int, expected: String, got: String) extends Problem(path, line) {
-        override def check(p: ConfigException.ValidationProblem) {
-            super.check(p)
-            val re = "List at.*" + path + ".*wrong value type.*expecting.*" + expected + ".*got.*element of.*" + got + ".*"
-            assertMessage(p, re)
-        }
-    }
-
-    private def checkException(e: ConfigException.ValidationFailed, expecteds: Seq[Problem]) {
-        val problems = e.problems().asScala.toIndexedSeq.sortBy(_.path).sortBy(_.origin.lineNumber)
-
-        //for (problem <- problems)
-        //    System.err.println(problem.origin().description() + ": " + problem.path() + ": " + problem.problem())
-
-        for ((problem, expected) <- problems zip expecteds) {
-            expected.check(problem)
-        }
-        assertEquals("found expected validation problems, got '" + problems + "' and expected '" + expecteds + "'",
-            expecteds.size, problems.size)
-    }
-
     @Test
     def validation() {
         val reference = ConfigFactory.parseFile(resourceFile("validate-reference.conf"), ConfigParseOptions.defaults())
@@ -87,7 +38,7 @@ class ValidationTest extends TestUtils {
             Missing("a.b.c.d.e.f.j", 28, "boolean"),
             WrongType("a.b.c.d.e.f.i", 30, "boolean", "list"))
 
-        checkException(e, expecteds)
+        checkValidationException(e, expecteds)
     }
 
     @Test
@@ -106,7 +57,7 @@ class ValidationTest extends TestUtils {
             Missing("a.b.c.d.e.f.j", 28, "boolean"),
             WrongType("a.b.c.d.e.f.i", 30, "boolean", "list"))
 
-        checkException(e, expecteds)
+        checkValidationException(e, expecteds)
     }
 
     @Test
@@ -130,7 +81,7 @@ class ValidationTest extends TestUtils {
 
         val expecteds = Seq(WrongType("a", 1, "list", "number"))
 
-        checkException(e, expecteds)
+        checkValidationException(e, expecteds)
     }
 
     @Test
@@ -143,7 +94,7 @@ class ValidationTest extends TestUtils {
 
         val expecteds = Seq(WrongElementType("a", 1, "boolean", "number"))
 
-        checkException(e, expecteds)
+        checkValidationException(e, expecteds)
     }
 
     @Test
@@ -163,7 +114,7 @@ class ValidationTest extends TestUtils {
 
         val expecteds = Seq(WrongType("a", 1, "list", "object"))
 
-        checkException(e, expecteds)
+        checkValidationException(e, expecteds)
     }
 
     @Test