From 24c6267362306f67ee633f68565edb07593e7b15 Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 22 Apr 2015 16:34:44 -0700 Subject: [PATCH 1/7] Fix substitute inside includes to respect lists --- .../typesafe/config/impl/ConfigParser.java | 20 +++++++----- .../typesafe/config/impl/ResolveSource.java | 31 +++++++++++++++++-- .../typesafe/config/impl/SimpleConfig.java | 3 +- .../src/test/resources/include-from-list.conf | 7 ++++- .../typesafe/config/impl/ConfParserTest.scala | 8 +++++ 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java index 8376a605..c5f526cd 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java @@ -190,10 +190,10 @@ final class ConfigParser { // 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 (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(); @@ -245,10 +245,10 @@ final class ConfigParser { // 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 ${}."); +// 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 @@ -356,6 +356,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,7 +378,9 @@ final class ConfigParser { values.add(v.withOrigin(v.origin().appendComments(new ArrayList(comments)))); comments.clear(); } + pathStack.push(new Path(Integer.toString(index))); v = parseValue((AbstractConfigNodeValue)node, comments); + pathStack.pop(); } } // There shouldn't be any comments at this point, but add them just in case diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index b192c305..beb64618 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -55,11 +55,15 @@ 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. - return findInObject(obj, path, null); + if (obj instanceof AbstractConfigObject) { + return findInObject((AbstractConfigObject) obj, path, null); + } else { + return findInList((SimpleConfigList) obj, path, null); + } } catch (ConfigException.NotResolved e) { throw ConfigImpl.improveNotResolved(path, e); } @@ -78,6 +82,29 @@ final class ResolveSource { } else { if (v instanceof AbstractConfigObject) { return findInObject((AbstractConfigObject) v, next, newParents); + } else if (v instanceof SimpleConfigList) { + return findInList((SimpleConfigList) v, next, newParents); + } else { + return new ValueWithPath(null, newParents); + } + } + } + + static private ValueWithPath findInList(SimpleConfigList list, Path path, Node parents) { + String key = path.first(); + Path next = path.remainder(); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("*** looking up '" + key + "' in " + list); + AbstractConfigValue v = list.get(Integer.parseInt(key)); + Node newParents = parents == null ? new Node(list) : parents.prepend(list); + + if (next == null) { + return new ValueWithPath(v, newParents); + } else { + if (v instanceof AbstractConfigObject) { + return findInObject((AbstractConfigObject) v, next, newParents); + } else if (v instanceof SimpleConfigList) { + return findInList((SimpleConfigList) v, next, newParents); } else { return new ValueWithPath(null, newParents); } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java index 2d76efb6..29c2795a 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java @@ -75,8 +75,9 @@ final class SimpleConfig implements Config, MergeableValue, Serializable { if (resolved == object) return this; - else + else { return new SimpleConfig((AbstractConfigObject) resolved); + } } private ConfigValue hasPathPeek(String pathExpression) { diff --git a/config/src/test/resources/include-from-list.conf b/config/src/test/resources/include-from-list.conf index 9f1c9bac..7d9ae87c 100644 --- a/config/src/test/resources/include-from-list.conf +++ b/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" + } + ] diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala index 75610212..c723e3ae 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala +++ b/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 includeWithSubstitutionsInArray() { + 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 From 0c6fd1551d5b0554f25ffcc74c64b75dfe48a2eb Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 22 Apr 2015 17:22:49 -0700 Subject: [PATCH 2/7] Cleanup and exception handling --- .../typesafe/config/impl/ResolveSource.java | 48 +++++++------------ .../config/impl/ConcatenationTest.scala | 11 +---- .../typesafe/config/impl/PublicApiTest.scala | 11 ----- 3 files changed, 20 insertions(+), 50 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index beb64618..6b864dd9 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -59,52 +59,40 @@ final class ResolveSource { try { // we'll fail if anything along the path can't // be looked at without resolving. - if (obj instanceof AbstractConfigObject) { - return findInObject((AbstractConfigObject) obj, path, null); - } else { - return findInList((SimpleConfigList) obj, path, null); - } + return findInObject(obj, path, null); } catch (ConfigException.NotResolved e) { throw ConfigImpl.improveNotResolved(path, e); } } - static private ValueWithPath findInObject(AbstractConfigObject obj, Path path, Node parents) { + static private ValueWithPath findInObject(AbstractConfigValue obj, Path path, Node 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 newParents = parents == null ? new Node(obj) : parents.prepend(obj); - if (next == null) { - return new ValueWithPath(v, newParents); - } else { - if (v instanceof AbstractConfigObject) { - return findInObject((AbstractConfigObject) v, next, newParents); - } else if (v instanceof SimpleConfigList) { - return findInList((SimpleConfigList) v, next, newParents); - } else { - return new ValueWithPath(null, newParents); + AbstractConfigValue v = null; + Node newParents = null; + + if (obj instanceof AbstractConfigObject) { + v = ((AbstractConfigObject) obj).attemptPeekWithPartialResolve(key); + newParents = parents == null ? new Node((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((SimpleConfigList) obj) : parents.prepend((SimpleConfigList) obj); } - } - - static private ValueWithPath findInList(SimpleConfigList list, Path path, Node parents) { - String key = path.first(); - Path next = path.remainder(); - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace("*** looking up '" + key + "' in " + list); - AbstractConfigValue v = list.get(Integer.parseInt(key)); - Node newParents = parents == null ? new Node(list) : parents.prepend(list); if (next == null) { return new ValueWithPath(v, newParents); } else { - if (v instanceof AbstractConfigObject) { - return findInObject((AbstractConfigObject) v, next, newParents); - } else if (v instanceof SimpleConfigList) { - return findInList((SimpleConfigList) v, next, newParents); + if (v instanceof AbstractConfigObject || v instanceof SimpleConfigList) { + return findInObject(v, next, newParents); } else { return new ValueWithPath(null, newParents); } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala index e3ca5412..deb70e7f 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala @@ -345,19 +345,12 @@ 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() - assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList) - } - assertTrue(e.getMessage.contains("limitation")) + 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) } - // 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] { diff --git a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala index 796c71de..e0bc395a 100644 --- a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala +++ b/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")) From 2ea5e2151184f97854b84ffe17ce95ba4206fce1 Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 22 Apr 2015 18:00:22 -0700 Subject: [PATCH 3/7] remove comments --- .../typesafe/config/impl/ConfigParser.java | 20 ------------------- .../config/impl/ConcatenationTest.scala | 4 ++-- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java index c5f526cd..65288306 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java @@ -187,14 +187,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); @@ -241,18 +233,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; } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala index deb70e7f..a21c8cb3 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala @@ -353,11 +353,11 @@ class ConcatenationTest extends TestUtils { @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 From 2506f745f7c36fe7d850a58475456acbcff26351 Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 22 Apr 2015 18:02:32 -0700 Subject: [PATCH 4/7] add test conf file and remove unneeded arrayCount --- .../com/typesafe/config/impl/ConfigParser.java | 17 ----------------- config/src/test/resources/test12.conf | 3 +++ 2 files changed, 3 insertions(+), 17 deletions(-) create mode 100644 config/src/test/resources/test12.conf diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java index 65288306..05cbdf1d 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java @@ -34,11 +34,6 @@ final class ConfigParser { final private ConfigOrigin baseOrigin; final private LinkedList 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(); - this.arrayCount = 0; } // merge a bunch of adjacent values into one @@ -95,8 +89,6 @@ final class ConfigParser { private AbstractConfigValue parseValue(AbstractConfigNodeValue n, List 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; } @@ -232,9 +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) { - arrayCount += 1; - } AbstractConfigNodeValue valueNode; AbstractConfigValue newValue; @@ -245,7 +231,6 @@ final class ConfigParser { newValue = parseValue(valueNode, comments); if (((ConfigNodeField) node).separator() == Tokens.PLUS_EQUALS) { - arrayCount -= 1; List concat = new ArrayList(2); AbstractConfigValue previousRef = new ConfigReference(newValue.origin(), @@ -326,7 +311,6 @@ final class ConfigParser { } private SimpleConfigList parseArray(ConfigNodeArray n) { - arrayCount += 1; SimpleConfigOrigin arrayOrigin = lineOrigin(); List values = new ArrayList(); @@ -367,7 +351,6 @@ final class ConfigParser { if (v != null) { values.add(v.withOrigin(v.origin().appendComments(new ArrayList(comments)))); } - arrayCount -= 1; return new SimpleConfigList(arrayOrigin, values); } diff --git a/config/src/test/resources/test12.conf b/config/src/test/resources/test12.conf new file mode 100644 index 00000000..1f0c551d --- /dev/null +++ b/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} \ No newline at end of file From b45345e3ac1aaae164035571e100de1160ad5a6b Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 22 Apr 2015 18:06:59 -0700 Subject: [PATCH 5/7] change test name and remove unnecessary braces --- .../src/main/java/com/typesafe/config/impl/SimpleConfig.java | 3 +-- .../test/scala/com/typesafe/config/impl/ConfParserTest.scala | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java index 29c2795a..2d76efb6 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java @@ -75,9 +75,8 @@ final class SimpleConfig implements Config, MergeableValue, Serializable { if (resolved == object) return this; - else { + else return new SimpleConfig((AbstractConfigObject) resolved); - } } private ConfigValue hasPathPeek(String pathExpression) { diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala index c723e3ae..1a7548df 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfParserTest.scala @@ -802,7 +802,7 @@ class ConfParserTest extends TestUtils { } @Test - def includeWithSubstitutionsInArray() { + def includeWithSubstitutionsFromList() { val conf = ConfigFactory.parseString("include file(" + jsonQuotedResourceFile("include-from-list") + ")") val resolved = conf.resolve() From 73520bea4c968ccfa93e0754155a46456c867b3b Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Wed, 29 Apr 2015 17:47:57 -0700 Subject: [PATCH 6/7] actually increment index --- config/src/main/java/com/typesafe/config/impl/ConfigParser.java | 1 + 1 file changed, 1 insertion(+) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java index 05cbdf1d..159b8659 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigParser.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigParser.java @@ -343,6 +343,7 @@ final class ConfigParser { comments.clear(); } pathStack.push(new Path(Integer.toString(index))); + index ++; v = parseValue((AbstractConfigNodeValue)node, comments); pathStack.pop(); } From fc4b3307767e509f9b67b092e0814e0a0484e263 Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Mon, 4 May 2015 14:43:49 -0700 Subject: [PATCH 7/7] Make note of list indexing in Hocon.md --- HOCON.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/HOCON.md b/HOCON.md index 944e9415..418310c8 100644 --- a/HOCON.md +++ b/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