Merge pull request #1 from twitter-forks/substitutions-in-includes

Substitutions in includes

.gitignore

@@ -1,6 +1,7 @@
 .classpath
 .project
 .cache
+.DS_Store
 .settings
 .idea
 .idea_modules

CONTRIBUTING.md

@@ -56,4 +56,4 @@ welcome!):
       release)
   16. push the "vX.Y.Z" tag
   17. announce release, possibly wait for maven central to sync
-      first
+      first

HOCON.md

@@ -1025,6 +1025,19 @@ Then the `${x}` in "foo.conf", which has been fixed up to
 `${a.x}`, would evaluate to `42` rather than to `10`.
 Substitution happens _after_ parsing the whole configuration.
 
+Includes inside of lists use the list index as a path segment when
+fixing up substitutions.
+
+Example:
+
+    {
+        a : [ { include "foo.conf" } ]
+        a.0.x : 42
+    }
+
+Since the `${x}` in "foo.conf" would be fixed up to `${a.0.x}' we
+get our expected value of 42.
+
 However, there are plenty of cases where the included file might
 intend to refer to the application's root config. For example, to
 get a value from a system property or from the reference

NEWS.md

@@ -382,4 +382,4 @@ Thank you to contributors with commits since v1.2.1 tag:
    `${foo}` syntax) without getting confused; just about anything
    that makes conceptual sense should now work. Only inherently
    circular config files should fail.
- - some minor documentation fixes.
+ - some minor documentation fixes.

README.md

@@ -832,4 +832,4 @@ format.
 
 #### Python port
 
-   * pyhocon https://github.com/chimpler/pyhocon
+   * pyhocon https://github.com/chimpler/pyhocon

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

@@ -252,4 +252,4 @@ public class ConfigBeanImpl {
 
         return false;
     }
-}
+}

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

@@ -339,4 +339,4 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl
             sb.append("# ) end of unresolved merge\n");
         }
     }
-}
+}

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

@@ -233,4 +233,4 @@ final public class ConfigImplUtil {
         }
         return nameBuilder.toString();
     }
-}
+}

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

@@ -278,4 +278,4 @@ final class ConfigNodeObject extends ConfigNodeComplexValue {
         Path path = PathParser.parsePathNode(desiredPath, flavor).value();
         return changeValueOnPath(path, null, flavor);
     }
-}
+}

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

@@ -34,11 +34,6 @@ final class ConfigParser {
         final private ConfigOrigin baseOrigin;
         final private LinkedList<Path> pathStack;
 
-        // the number of lists we are inside; this is used to detect the "cannot
-        // generate a reference to a list element" problem, and once we fix that
-        // problem we should be able to get rid of this variable.
-        int arrayCount;
-
         ParseContext(ConfigSyntax flavor, ConfigOrigin origin, ConfigNodeRoot document,
                 FullIncluder includer, ConfigIncludeContext includeContext) {
             lineNumber = 1;
@@ -48,7 +43,6 @@ final class ConfigParser {
             this.includer = includer;
             this.includeContext = includeContext;
             this.pathStack = new LinkedList<Path>();
-            this.arrayCount = 0;
         }
 
         // merge a bunch of adjacent values into one
@@ -95,8 +89,6 @@ final class ConfigParser {
         private AbstractConfigValue parseValue(AbstractConfigNodeValue n, List<String> comments) {
             AbstractConfigValue v;
 
-            int startingArrayCount = arrayCount;
-
             if (n instanceof ConfigNodeSimpleValue) {
                 v = ((ConfigNodeSimpleValue) n).value();
             } else if (n instanceof ConfigNodeObject) {
@@ -114,9 +106,6 @@ final class ConfigParser {
                 comments.clear();
             }
 
-            if (arrayCount != startingArrayCount)
-                throw new ConfigException.BugOrBroken("Bug in config parser: unbalanced array count");
-
             return v;
         }
 
@@ -187,14 +176,6 @@ final class ConfigParser {
                     throw new ConfigException.BugOrBroken("should not be reached");
             }
 
-            // we really should make this work, but for now throwing an
-            // exception is better than producing an incorrect result.
-            // See https://github.com/typesafehub/config/issues/160
-            if (arrayCount > 0 && obj.resolveStatus() != ResolveStatus.RESOLVED)
-                throw parseError("Due to current limitations of the config parser, when an include statement is nested inside a list value, "
-                        + "${} substitutions inside the included file cannot be resolved correctly. Either move the include outside of the list value or "
-                        + "remove the ${} statements from the included file.");
-
             if (!pathStack.isEmpty()) {
                 Path prefix = fullCurrentPath();
                 obj = obj.relativized(prefix);
@@ -240,21 +221,6 @@ final class ConfigParser {
 
                     // path must be on-stack while we parse the value
                     pathStack.push(path);
-                    if (((ConfigNodeField) node).separator() == Tokens.PLUS_EQUALS) {
-                        // we really should make this work, but for now throwing
-                        // an exception is better than producing an incorrect
-                        // result. See
-                        // https://github.com/typesafehub/config/issues/160
-                        if (arrayCount > 0)
-                            throw parseError("Due to current limitations of the config parser, += does not work nested inside a list. "
-                                    + "+= expands to a ${} substitution and the path in ${} cannot currently refer to list elements. "
-                                    + "You might be able to move the += outside of the list and then refer to it from inside the list with ${}.");
-
-                        // because we will put it in an array after the fact so
-                        // we want this to be incremented during the parseValue
-                        // below in order to throw the above exception.
-                        arrayCount += 1;
-                    }
 
                     AbstractConfigNodeValue valueNode;
                     AbstractConfigValue newValue;
@@ -265,7 +231,6 @@ final class ConfigParser {
                     newValue = parseValue(valueNode, comments);
 
                     if (((ConfigNodeField) node).separator() == Tokens.PLUS_EQUALS) {
-                        arrayCount -= 1;
 
                         List<AbstractConfigValue> concat = new ArrayList<AbstractConfigValue>(2);
                         AbstractConfigValue previousRef = new ConfigReference(newValue.origin(),
@@ -346,7 +311,6 @@ final class ConfigParser {
         }
 
         private SimpleConfigList parseArray(ConfigNodeArray n) {
-            arrayCount += 1;
 
             SimpleConfigOrigin arrayOrigin = lineOrigin();
             List<AbstractConfigValue> values = new ArrayList<AbstractConfigValue>();
@@ -356,6 +320,8 @@ final class ConfigParser {
 
             AbstractConfigValue v = null;
 
+            int index = 0;
+
             for (AbstractConfigNode node : n.children()) {
                 if (node instanceof ConfigNodeComment) {
                     comments.add(((ConfigNodeComment) node).commentText());
@@ -376,14 +342,16 @@ final class ConfigParser {
                         values.add(v.withOrigin(v.origin().appendComments(new ArrayList<String>(comments))));
                         comments.clear();
                     }
+                    pathStack.push(new Path(Integer.toString(index)));
+                    index ++;
                     v = parseValue((AbstractConfigNodeValue)node, comments);
+                    pathStack.pop();
                 }
             }
             // There shouldn't be any comments at this point, but add them just in case
             if (v != null) {
                 values.add(v.withOrigin(v.origin().appendComments(new ArrayList<String>(comments))));
             }
-            arrayCount -= 1;
             return new SimpleConfigList(arrayOrigin, values);
         }
 

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

@@ -55,7 +55,7 @@ final class ResolveSource {
         }
     }
 
-    static private ValueWithPath findInObject(AbstractConfigObject obj, Path path) {
+    static private ValueWithPath findInObject(AbstractConfigValue obj, Path path) {
         try {
             // we'll fail if anything along the path can't
             // be looked at without resolving.
@@ -65,19 +65,34 @@ final class ResolveSource {
         }
     }
 
-    static private ValueWithPath findInObject(AbstractConfigObject obj, Path path, Node<Container> parents) {
+    static private ValueWithPath findInObject(AbstractConfigValue obj, Path path, Node<Container> parents) {
         String key = path.first();
         Path next = path.remainder();
         if (ConfigImpl.traceSubstitutionsEnabled())
             ConfigImpl.trace("*** looking up '" + key + "' in " + obj);
-        AbstractConfigValue v = obj.attemptPeekWithPartialResolve(key);
+
-        Node<Container> newParents = parents == null ? new Node<Container>(obj) : parents.prepend(obj);
+        AbstractConfigValue v = null;
+        Node<Container> newParents = null;
+
+        if (obj instanceof AbstractConfigObject) {
+            v = ((AbstractConfigObject) obj).attemptPeekWithPartialResolve(key);
+            newParents = parents == null ? new Node<Container>((AbstractConfigObject) obj) : parents.prepend((AbstractConfigObject) obj);
+        } else if (obj instanceof SimpleConfigList) {
+            int ix;
+            try {
+                ix = Integer.parseInt(key);
+            } catch(Exception e) {
+                throw new ConfigException.BadPath(key, "path for list must be numeric index");
+            }
+            v = ((SimpleConfigList) obj).get(ix);
+            newParents = parents == null ? new Node<Container>((SimpleConfigList) obj) : parents.prepend((SimpleConfigList) obj);
+        }
 
         if (next == null) {
             return new ValueWithPath(v, newParents);
         } else {
-            if (v instanceof AbstractConfigObject) {
+            if (v instanceof AbstractConfigObject || v instanceof SimpleConfigList) {
-                return findInObject((AbstractConfigObject) v, next, newParents);
+                return findInObject(v, next, newParents);
             } else {
                 return new ValueWithPath(null, newParents);
             }

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

@@ -60,4 +60,4 @@ final class SimpleConfigDocument implements ConfigDocument {
     public int hashCode() {
         return render().hashCode();
     }
-}
+}

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

@@ -692,4 +692,4 @@ final class Tokenizer {
                     "Does not make sense to remove items from token stream");
         }
     }
-}
+}

config/src/test/java/beanconfig/ArraysConfig.java

@@ -136,4 +136,4 @@ public class ArraysConfig {
     public void setOfStringBean(List<StringsConfig> ofStringBean) {
         this.ofStringBean = ofStringBean;
     }
-}
+}

config/src/test/java/beanconfig/StringsConfig.java

@@ -42,4 +42,4 @@ public class StringsConfig {
     public String toString() {
         return "StringsConfig(" + abcd + "," + yes + ")";
     }
-}
+}

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

@@ -52,17 +52,7 @@
         "ofConfigObject" : [${numbers}, ${booleans}, ${strings}],
         "ofConfigValue" : [1, 2, "a"],
         "ofDuration" : [1, 2h, 3 days],
-        "ofMemorySize" : [1024, 1M, 1G],
+        "ofMemorySize" : [1024, 1M, 1G]
-        "ofStringBean" : [
-                {
-                        abcd : "testAbcdOne"
-                        yes : "testYesOne"
-                },
-                {
-                        abcd : "testAbcdTwo"
-                        yes : "testYesTwo"
-                }
-        ]
     },
     "bytes" : {
         "kilobyte" : "1kB",

config/src/test/resources/include-from-list.conf

@@ -1,5 +1,10 @@
 // The {} inside the [] is needed because
 // just [ include ] means an array with the
 // string "include" in it.
-a = [ { include "test01.conf" } ]
+a = [
+    {
+       include "test12.conf"
+       replace-key: "replaced"
+     }
+ ]
 

config/src/test/resources/test12.conf

@@ -0,0 +1,3 @@
+// This file is included by include-from-list.conf to verify that includes with substitutions work in arrays
+
+replace-me: ${replace-key}

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

@@ -345,26 +345,19 @@ class ConcatenationTest extends TestUtils {
         assertEquals(Seq(1, 2, 3), conf.getObjectList("x.a").asScala.toList.map(_.toConfig.getInt("b")))
     }
 
-    // We would ideally make this case NOT throw an exception but we need to do some work
-    // to get there, see https://github.com/typesafehub/config/issues/160
     @Test
     def plusEqualsMultipleTimesNestedInArray() {
-        val e = intercept[ConfigException.Parse] {
+        val conf = parseConfig("""x = [ { a += 1, a += 2, a += 3 } ] """).resolve()
-            val conf = parseConfig("""x = [ { a += 1, a += 2, a += 3 } ] """).resolve()
+        assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList)
-            assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList)
-        }
-        assertTrue(e.getMessage.contains("limitation"))
     }
 
-    // We would ideally make this case NOT throw an exception but we need to do some work
-    // to get there, see https://github.com/typesafehub/config/issues/160
     @Test
     def plusEqualsMultipleTimesNestedInPlusEquals() {
-        val e = intercept[ConfigException.Parse] {
+        val e = intercept[ConfigException.BugOrBroken] {
             val conf = parseConfig("""x += { a += 1, a += 2, a += 3 } """).resolve()
             assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList)
         }
-        assertTrue(e.getMessage.contains("limitation"))
+        assertTrue(e.getMessage.contains("did not find"))
     }
 
     // from https://github.com/typesafehub/config/issues/177

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

@@ -801,6 +801,14 @@ class ConfParserTest extends TestUtils {
         assertTrue("including basename URL doesn't load anything", conf.isEmpty())
     }
 
+    @Test
+    def includeWithSubstitutionsFromList() {
+        val conf = ConfigFactory.parseString("include file(" + jsonQuotedResourceFile("include-from-list") + ")")
+
+        val resolved = conf.resolve()
+        assertEquals(resolved.getConfigList("a").get(0).getString("replace-me"), "replaced")
+    }
+
     @Test
     def acceptBOMStartingFile() {
         // BOM at start of file should be ignored

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

@@ -209,4 +209,4 @@ class ConfigBeanFactoryTest extends TestUtils {
         }
     }
 
-}
+}

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

@@ -455,4 +455,4 @@ class ConfigDocumentTest extends TestUtils {
         assertEquals("{ a : {\n     \"a\" : 1,\n     \"b\" : 2\n } }",
             configDocument.withValue("a", configVal).render)
     }
-}
+}

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

@@ -229,4 +229,4 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(finalText, newNode.render())
     }
 
-}
+}

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

@@ -875,17 +875,6 @@ class PublicApiTest extends TestUtils {
         assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("include statements nested"))
     }
 
-    // We would ideally make this case NOT throw an exception but we need to do some work
-    // to get there, see https://github.com/typesafehub/config/issues/160
-    @Test
-    def detectIncludeFromList() {
-        val e = intercept[ConfigException.Parse] {
-            ConfigFactory.load("include-from-list.conf")
-        }
-
-        assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("limitation"))
-    }
-
     @Test
     def missingOverrideResourceFails() {
         assertEquals("config.file is not set", null, System.getProperty("config.file"))