From bfe83e1b7992f7654a9fcba46a5bae87131beef6 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Thu, 1 May 2014 11:19:39 -0400 Subject: [PATCH] Throw exceptions when we need a reference to an array element This arose in issue https://github.com/typesafehub/config/issues/160 There are two known cases: when we want to relativize references in included files, and when we use `+=` while inside a list. Both of those are currently impossible to handle without a way to generate a reference to a list element. --- .../java/com/typesafe/config/impl/Parser.java | 42 ++++++++++++++++ .../src/test/resources/include-from-list.conf | 5 ++ .../config/impl/ConcatenationTest.scala | 48 +++++++++++++++++++ .../typesafe/config/impl/PublicApiTest.scala | 11 +++++ 4 files changed, 106 insertions(+) create mode 100644 config/src/test/resources/include-from-list.conf diff --git a/config/src/main/java/com/typesafe/config/impl/Parser.java b/config/src/main/java/com/typesafe/config/impl/Parser.java index 4969255ad..aafca6302 100644 --- a/config/src/main/java/com/typesafe/config/impl/Parser.java +++ b/config/src/main/java/com/typesafe/config/impl/Parser.java @@ -126,6 +126,10 @@ static private final class ParseContext { // used to modify the error message to reflect that // someone may think this is .properties format. int equalsCount; + // 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, Iterator tokens, FullIncluder includer, ConfigIncludeContext includeContext) { @@ -138,6 +142,7 @@ static private final class ParseContext { this.includeContext = includeContext; this.pathStack = new LinkedList(); this.equalsCount = 0; + this.arrayCount = 0; } static private boolean attractsTrailingComments(Token token) { @@ -499,6 +504,9 @@ private String addQuoteSuggestion(Path lastPath, boolean insideEquals, String ba private AbstractConfigValue parseValue(TokenWithComments t) { AbstractConfigValue v; + int startingArrayCount = arrayCount; + int startingEqualsCount = equalsCount; + if (Tokens.isValue(t.token)) { // if we consolidateValueTokens() multiple times then // this value could be a concatenation, object, array, @@ -519,6 +527,11 @@ private AbstractConfigValue parseValue(TokenWithComments t) { v = v.withOrigin(t.prependComments(v.origin())); + if (arrayCount != startingArrayCount) + throw new ConfigException.BugOrBroken("Bug in config parser: unbalanced array count"); + if (equalsCount != startingEqualsCount) + throw new ConfigException.BugOrBroken("Bug in config parser: unbalanced equals count"); + return v; } @@ -678,6 +691,14 @@ private void parseInclude(Map values) { throw parseError("include keyword is not followed by a quoted string, but by: " + t); } + // 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); @@ -739,6 +760,21 @@ private AbstractConfigObject parseObject(boolean hadOpenCurly) { // path must be on-stack while we parse the value pathStack.push(path); + if (afterKey.token == 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; + } TokenWithComments valueToken; AbstractConfigValue newValue; @@ -767,6 +803,8 @@ private AbstractConfigObject parseObject(boolean hadOpenCurly) { newValue = parseValue(valueToken.prepend(keyToken.comments)); if (afterKey.token == Tokens.PLUS_EQUALS) { + arrayCount -= 1; + List concat = new ArrayList(2); AbstractConfigValue previousRef = new ConfigReference(newValue.origin(), new SubstitutionExpression(fullCurrentPath(), true /* optional */)); @@ -858,6 +896,8 @@ private AbstractConfigObject parseObject(boolean hadOpenCurly) { private SimpleConfigList parseArray() { // invoked just after the OPEN_SQUARE + arrayCount += 1; + SimpleConfigOrigin arrayOrigin = lineOrigin(); List values = new ArrayList(); @@ -867,6 +907,7 @@ private SimpleConfigList parseArray() { // special-case the first element if (t.token == Tokens.CLOSE_SQUARE) { + arrayCount -= 1; return new SimpleConfigList(t.appendComments(arrayOrigin), Collections. emptyList()); } else if (Tokens.isValue(t.token) || t.token == Tokens.OPEN_CURLY @@ -890,6 +931,7 @@ private SimpleConfigList parseArray() { } else { t = nextTokenIgnoringNewline(); if (t.token == Tokens.CLOSE_SQUARE) { + arrayCount -= 1; return new SimpleConfigList(t.appendComments(arrayOrigin), values); } else { throw parseError(addKeyName("List should have ended with ] or had a comma, instead had token: " diff --git a/config/src/test/resources/include-from-list.conf b/config/src/test/resources/include-from-list.conf new file mode 100644 index 000000000..9f1c9bacf --- /dev/null +++ b/config/src/test/resources/include-from-list.conf @@ -0,0 +1,5 @@ +// The {} inside the [] is needed because +// just [ include ] means an array with the +// string "include" in it. +a = [ { include "test01.conf" } ] + 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 c2a3fc709..95b82db82 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala @@ -320,4 +320,52 @@ class ConcatenationTest extends TestUtils { val conf = parseConfig(""" a = ${x}, a += ${y}, x = [1], y = 2 """).resolve() assertEquals(Seq(1, 2), conf.getIntList("a").asScala.toList) } + + @Test + def plusEqualsMultipleTimes() { + val conf = parseConfig(""" a += 1, a += 2, a += 3 """).resolve() + assertEquals(Seq(1, 2, 3), conf.getIntList("a").asScala.toList) + } + + @Test + def plusEqualsMultipleTimesNested() { + val conf = parseConfig(""" x { a += 1, a += 2, a += 3 } """).resolve() + assertEquals(Seq(1, 2, 3), conf.getIntList("x.a").asScala.toList) + } + + @Test + def plusEqualsAnObjectMultipleTimes() { + val conf = parseConfig(""" a += { b: 1 }, a += { b: 2 }, a += { b: 3 } """).resolve() + assertEquals(Seq(1, 2, 3), conf.getObjectList("a").asScala.toList.map(_.toConfig.getInt("b"))) + } + + @Test + def plusEqualsAnObjectMultipleTimesNested() { + val conf = parseConfig(""" x { a += { b: 1 }, a += { b: 2 }, a += { b: 3 } } """).resolve() + 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")) + } + + // 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() { + System.err.println("==============") + 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")) + System.err.println("==============") + } } 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 64a2e2ef9..2e4e26242 100644 --- a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala @@ -827,6 +827,17 @@ 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"))