Merge pull request #102 from typesafehub/wip/havocp-checkvalid-indexed-objects

Fix for #101 checkValid handling of indexed objects

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

@@ -339,6 +339,11 @@ public abstract class ConfigException extends RuntimeException implements Serial
         public String problem() {
             return problem;
         }
+
+        @Override
+        public String toString() {
+            return "ValidationProblem(" + path + "," + origin + "," + problem + ")";
+        }
     }
 
     /**

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

@@ -728,7 +728,8 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
                 return false;
             }
         } else if (reference instanceof SimpleConfigList) {
-            if (value instanceof SimpleConfigList) {
+            // objects may be convertible to lists if they have numeric keys
+            if (value instanceof SimpleConfigList || value instanceof SimpleConfigObject) {
                 return true;
             } else {
                 return false;
@@ -772,6 +773,25 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
         }
     }
 
+    private static void checkListCompatibility(Path path, SimpleConfigList listRef,
+            SimpleConfigList listValue, List<ConfigException.ValidationProblem> accumulator) {
+        if (listRef.isEmpty() || listValue.isEmpty()) {
+            // can't verify type, leave alone
+        } else {
+            AbstractConfigValue refElement = listRef.get(0);
+            for (ConfigValue elem : listValue) {
+                AbstractConfigValue e = (AbstractConfigValue) elem;
+                if (!haveCompatibleTypes(refElement, e)) {
+                    addProblem(accumulator, path, e.origin(), "List at '" + path.render()
+                            + "' contains wrong value type, expecting list of "
+                            + getDesc(refElement) + " but got element of type " + getDesc(e));
+                    // don't add a problem for every last array element
+                    break;
+                }
+            }
+        }
+    }
+
     private static void checkValid(Path path, ConfigValue reference, AbstractConfigValue value,
             List<ConfigException.ValidationProblem> accumulator) {
         // Unmergeable is supposed to be impossible to encounter in here
@@ -784,22 +804,16 @@ final class SimpleConfig implements Config, MergeableValue, Serializable {
             } else if (reference instanceof SimpleConfigList && value instanceof SimpleConfigList) {
                 SimpleConfigList listRef = (SimpleConfigList) reference;
                 SimpleConfigList listValue = (SimpleConfigList) value;
-                if (listRef.isEmpty() || listValue.isEmpty()) {
+                checkListCompatibility(path, listRef, listValue, accumulator);
-                    // can't verify type, leave alone
+            } else if (reference instanceof SimpleConfigList && value instanceof SimpleConfigObject) {
-                } else {
+                // attempt conversion of indexed object to list
-                    AbstractConfigValue refElement = listRef.get(0);
+                SimpleConfigList listRef = (SimpleConfigList) reference;
-                    for (ConfigValue elem : listValue) {
+                AbstractConfigValue listValue = DefaultTransformer.transform(value,
-                        AbstractConfigValue e = (AbstractConfigValue) elem;
+                        ConfigValueType.LIST);
-                        if (!haveCompatibleTypes(refElement, e)) {
+                if (listValue instanceof SimpleConfigList)
-                            addProblem(accumulator, path, e.origin(), "List at '" + path.render()
+                    checkListCompatibility(path, listRef, (SimpleConfigList) listValue, accumulator);
-                                    + "' contains wrong value type, expecting list of "
+                else
-                                    + getDesc(refElement) + " but got element of type "
+                    addWrongType(accumulator, reference, value, path);
-                                    + getDesc(e));
-                            // don't add a problem for every last array element
-                            break;
-                        }
-                    }
-                }
             }
         } else {
             addWrongType(accumulator, reference, value, path);

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

@@ -107,7 +107,9 @@ class PropertiesTest extends TestUtils {
         props.setProperty("a.4", "4")
 
         val conf = ConfigFactory.parseProperties(props, ConfigParseOptions.defaults())
+        val reference = ConfigFactory.parseString("{ a : [0,1,2,3,4] }")
         assertEquals(Seq(0, 1, 2, 3, 4), conf.getIntList("a").asScala.toSeq)
+        conf.checkValid(reference)
     }
 
     @Test
@@ -120,7 +122,9 @@ class PropertiesTest extends TestUtils {
         props.setProperty("a.4", "2")
 
         val conf = ConfigFactory.parseProperties(props, ConfigParseOptions.defaults())
+        val reference = ConfigFactory.parseString("{ a : [0,1,2] }")
         assertEquals(Seq(0, 1, 2), conf.getIntList("a").asScala.toSeq)
+        conf.checkValid(reference)
     }
 
     @Test
@@ -137,7 +141,9 @@ class PropertiesTest extends TestUtils {
         props.setProperty("a.4", "4")
 
         val conf = ConfigFactory.parseProperties(props, ConfigParseOptions.defaults())
+        val reference = ConfigFactory.parseString("{ a : [0,1,2,3,4] }")
         assertEquals(Seq(0, 1, 2, 3, 4), conf.getIntList("a").asScala.toSeq)
+        conf.checkValid(reference)
     }
 
     @Test
@@ -173,7 +179,9 @@ class PropertiesTest extends TestUtils {
                 a = [-2, -1] ${a}
                 """)
         val conf = conf2.withFallback(conf1).resolve()
+        val reference = ConfigFactory.parseString("{ a : [-2,-1,0,1,2,3,4,5,6] }")
 
         assertEquals(Seq(-2, -1, 0, 1, 2, 3, 4, 5, 6), conf.getIntList("a").asScala.toSeq)
+        conf.checkValid(reference)
     }
 }

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

@@ -15,8 +15,8 @@ class ValidationTest extends TestUtils {
 
     sealed abstract class Problem(path: String, line: Int) {
         def check(p: ConfigException.ValidationProblem) {
-            assertEquals(path, p.path())
+            assertEquals("matching path", path, p.path())
-            assertEquals(line, p.origin().lineNumber())
+            assertEquals("matching line", line, p.origin().lineNumber())
         }
 
         protected def assertMessage(p: ConfigException.ValidationProblem, re: String) {
@@ -58,7 +58,8 @@ class ValidationTest extends TestUtils {
         for ((problem, expected) <- problems zip expecteds) {
             expected.check(problem)
         }
-        assertEquals(expecteds.size, problems.size)
+        assertEquals("found expected validation problems, got '" + problems + "' and expected '" + expecteds + "'",
+            expecteds.size, problems.size)
     }
 
     @Test
@@ -118,4 +119,59 @@ class ValidationTest extends TestUtils {
         assertTrue("expected different message, got: " + e.getMessage,
             e.getMessage.contains("resolve"))
     }
+
+    @Test
+    def validationCatchesListOverriddenWithNumber() {
+        val reference = parseConfig("""{ a : [{},{},{}] }""")
+        val conf = parseConfig("""{ a : 42 }""")
+        val e = intercept[ConfigException.ValidationFailed] {
+            conf.checkValid(reference)
+        }
+
+        val expecteds = Seq(WrongType("a", 1, "list", "number"))
+
+        checkException(e, expecteds)
+    }
+
+    @Test
+    def validationCatchesListOverriddenWithDifferentList() {
+        val reference = parseConfig("""{ a : [true,false,false] }""")
+        val conf = parseConfig("""{ a : [42,43] }""")
+        val e = intercept[ConfigException.ValidationFailed] {
+            conf.checkValid(reference)
+        }
+
+        val expecteds = Seq(WrongElementType("a", 1, "boolean", "number"))
+
+        checkException(e, expecteds)
+    }
+
+    @Test
+    def validationAllowsListOverriddenWithSameTypeList() {
+        val reference = parseConfig("""{ a : [1,2,3] }""")
+        val conf = parseConfig("""{ a : [4,5] }""")
+        conf.checkValid(reference)
+    }
+
+    @Test
+    def validationCatchesListOverriddenWithNoIndexesObject() {
+        val reference = parseConfig("""{ a : [1,2,3] }""")
+        val conf = parseConfig("""{ a : { notANumber: foo } }""")
+        val e = intercept[ConfigException.ValidationFailed] {
+            conf.checkValid(reference)
+        }
+
+        val expecteds = Seq(WrongType("a", 1, "list", "object"))
+
+        checkException(e, expecteds)
+    }
+
+    @Test
+    def validationAllowsListOverriddenWithIndexedObject() {
+        val reference = parseConfig("""{ a : [a,b,c] }""")
+        val conf = parseConfig("""{ a : { "0" : x, "1" : y } }""")
+        conf.checkValid(reference)
+        assertEquals("got the sequence from overriding list with indexed object",
+            Seq("x", "y"), conf.getStringList("a").asScala)
+    }
 }