From b0fdc6456ca81a178628b6fa13f1dff0ef9e61f7 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sat, 12 Nov 2011 19:23:09 -0500 Subject: [PATCH] Relativize substitutions when including a file This is needed because we resolve substitutions only as the last step, and against the whole tree not against each file. Special handling is needed so that fallback to system properties and environment variables still works properly. --- .../config/impl/AbstractConfigObject.java | 57 +++++++++++++------ .../config/impl/AbstractConfigValue.java | 20 +++++++ .../config/impl/ConfigDelayedMerge.java | 9 +++ .../config/impl/ConfigDelayedMergeObject.java | 9 +++ .../config/impl/ConfigSubstitution.java | 37 ++++++++++-- .../java/com/typesafe/config/impl/Parser.java | 13 +++++ .../java/com/typesafe/config/impl/Path.java | 49 ++++++++++++++++ .../com/typesafe/config/impl/PathBuilder.java | 18 ++++++ .../config/impl/SimpleConfigList.java | 44 ++++++++++---- src/test/resources/test01.conf | 12 ++++ .../config/impl/ConfigSubstitutionTest.scala | 25 ++++++++ .../com/typesafe/config/impl/ConfigTest.scala | 34 +++++++++++ .../com/typesafe/config/impl/PathTest.scala | 20 +++++++ 13 files changed, 314 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index c55253f2..941b2005 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -270,40 +270,63 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements } } - @Override - AbstractConfigObject resolveSubstitutions(SubstitutionResolver resolver, - int depth, - boolean withFallbacks) { - if (resolveStatus() == ResolveStatus.RESOLVED) - return this; - + private AbstractConfigObject modify(Modifier modifier, + ResolveStatus newResolveStatus) { Map changes = null; for (String k : keySet()) { AbstractConfigValue v = peek(k); - AbstractConfigValue resolved = resolver.resolve(v, depth, - withFallbacks); - if (resolved != v) { + AbstractConfigValue modified = modifier.modifyChild(v); + if (modified != v) { if (changes == null) changes = new HashMap(); - changes.put(k, resolved); + changes.put(k, modified); } } if (changes == null) { - return newCopy(transformer, ResolveStatus.RESOLVED); + return newCopy(transformer, newResolveStatus); } else { - Map resolved = new HashMap(); + Map modified = new HashMap(); for (String k : keySet()) { if (changes.containsKey(k)) { - resolved.put(k, changes.get(k)); + modified.put(k, changes.get(k)); } else { - resolved.put(k, peek(k)); + modified.put(k, peek(k)); } } - return new SimpleConfigObject(origin(), transformer, resolved, - ResolveStatus.RESOLVED); + return new SimpleConfigObject(origin(), transformer, modified, + newResolveStatus); } } + @Override + AbstractConfigObject resolveSubstitutions(final SubstitutionResolver resolver, + final int depth, + final boolean withFallbacks) { + if (resolveStatus() == ResolveStatus.RESOLVED) + return this; + + return modify(new Modifier() { + + @Override + public AbstractConfigValue modifyChild(AbstractConfigValue v) { + return resolver.resolve(v, depth, withFallbacks); + } + + }, ResolveStatus.RESOLVED); + } + + @Override + AbstractConfigObject relativized(final Path prefix) { + return modify(new Modifier() { + + @Override + public AbstractConfigValue modifyChild(AbstractConfigValue v) { + return v.relativized(prefix); + } + + }, resolveStatus()); + } + @Override public AbstractConfigValue get(Object key) { if (key instanceof String) diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index 5b227f70..12b6f3b6 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -38,6 +38,26 @@ abstract class AbstractConfigValue implements ConfigValue { return ResolveStatus.RESOLVED; } + /** + * This is used when including one file in another; the included file is + * relativized to the path it's included into in the parent file. The point + * is that if you include a file at foo.bar in the parent, and the included + * file as a substitution ${a.b.c}, the included substitution now needs to + * be ${foo.bar.a.b.c} because we resolve substitutions globally only after + * parsing everything. + * + * @param prefix + * @return value relativized to the given path or the same value if nothing + * to do + */ + AbstractConfigValue relativized(Path prefix) { + return this; + } + + protected interface Modifier { + AbstractConfigValue modifyChild(AbstractConfigValue v); + } + @Override public AbstractConfigValue withFallback(ConfigValue other) { return this; diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java index 76d47940..414eb6d3 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -90,6 +90,15 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements return ResolveStatus.UNRESOLVED; } + @Override + ConfigDelayedMerge relativized(Path prefix) { + List newStack = new ArrayList(); + for (AbstractConfigValue o : stack) { + newStack.add(o.relativized(prefix)); + } + return new ConfigDelayedMerge(origin(), newStack); + } + @Override public AbstractConfigValue withFallback(ConfigValue other) { if (other instanceof AbstractConfigObject diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java index 6c9fcad7..f75a8c2a 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -91,6 +91,15 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements return ResolveStatus.UNRESOLVED; } + @Override + ConfigDelayedMergeObject relativized(Path prefix) { + List newStack = new ArrayList(); + for (AbstractConfigValue o : stack) { + newStack.add(o.relativized(prefix)); + } + return new ConfigDelayedMergeObject(origin(), transformer, newStack); + } + @Override public ConfigDelayedMergeObject withFallback(ConfigValue other) { if (other instanceof AbstractConfigObject diff --git a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java index 2cfe5064..20b1a89e 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -22,10 +22,18 @@ final class ConfigSubstitution extends AbstractConfigValue implements // have to be resolved to values, then if there's more // than one piece everything is stringified and concatenated final private List pieces; + // the length of any prefixes added with relativized() + final int prefixLength; ConfigSubstitution(ConfigOrigin origin, List pieces) { + this(origin, pieces, 0); + } + + private ConfigSubstitution(ConfigOrigin origin, List pieces, + int prefixLength) { super(origin); this.pieces = pieces; + this.prefixLength = prefixLength; } @Override @@ -104,15 +112,17 @@ final class ConfigSubstitution extends AbstractConfigValue implements ConfigValue result = findInObject(resolver.root(), resolver, subst, depth, withFallbacks); if (withFallbacks) { + // when looking up system props and env variables, + // we don't want the prefix that was added when + // we were included in another file. + Path unprefixed = subst.subPath(prefixLength); if (result == null) { result = findInObject(ConfigImpl.systemPropertiesConfig(), - null, - subst, depth, withFallbacks); + null, unprefixed, depth, withFallbacks); } if (result == null) { result = findInObject(ConfigImpl.envVariablesConfig(), null, - subst, - depth, withFallbacks); + unprefixed, depth, withFallbacks); } } if (result == null) { @@ -173,6 +183,25 @@ final class ConfigSubstitution extends AbstractConfigValue implements return ResolveStatus.UNRESOLVED; } + // when you graft a substitution into another object, + // you have to prefix it with the location in that object + // where you grafted it; but save prefixLength so + // system property and env variable lookups don't get + // broken. + @Override + ConfigSubstitution relativized(Path prefix) { + List newPieces = new ArrayList(); + for (Object p : pieces) { + if (p instanceof Path) { + newPieces.add(((Path) p).prepend(prefix)); + } else { + newPieces.add(p); + } + } + return new ConfigSubstitution(origin(), newPieces, prefixLength + + prefix.length()); + } + @Override protected boolean canEqual(Object other) { return other instanceof ConfigSubstitution; diff --git a/src/main/java/com/typesafe/config/impl/Parser.java b/src/main/java/com/typesafe/config/impl/Parser.java index 2d8392c1..f1d182cb 100644 --- a/src/main/java/com/typesafe/config/impl/Parser.java +++ b/src/main/java/com/typesafe/config/impl/Parser.java @@ -14,6 +14,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.ListIterator; import java.util.Map; @@ -118,6 +119,7 @@ final class Parser { final private IncludeHandler includer; final private SyntaxFlavor flavor; final private ConfigOrigin baseOrigin; + final private LinkedList pathStack; ParseContext(SyntaxFlavor flavor, ConfigOrigin origin, Iterator tokens, IncludeHandler includer) { @@ -127,6 +129,7 @@ final class Parser { this.flavor = flavor; this.baseOrigin = origin; this.includer = includer; + this.pathStack = new LinkedList(); } private Token nextToken() { @@ -397,6 +400,11 @@ final class Parser { String name = (String) Tokens.getValue(t).unwrapped(); AbstractConfigObject obj = includer.include(name); + if (!pathStack.isEmpty()) { + Path prefix = new Path(pathStack); + obj = obj.relativized(prefix); + } + for (String key : obj.keySet()) { AbstractConfigValue v = obj.get(key); AbstractConfigValue existing = values.get(key); @@ -446,6 +454,9 @@ final class Parser { Path path = parseKey(t); Token afterKey = nextTokenIgnoringNewline(); + // path must be on-stack while we parse the value + pathStack.push(path); + Token valueToken; AbstractConfigValue newValue; if (flavor == SyntaxFlavor.CONF @@ -464,6 +475,8 @@ final class Parser { newValue = parseValue(valueToken); } + pathStack.pop(); + String key = path.first(); Path remaining = path.remainder(); diff --git a/src/main/java/com/typesafe/config/impl/Path.java b/src/main/java/com/typesafe/config/impl/Path.java index 7c67fc45..6d77979e 100644 --- a/src/main/java/com/typesafe/config/impl/Path.java +++ b/src/main/java/com/typesafe/config/impl/Path.java @@ -1,5 +1,8 @@ package com.typesafe.config.impl; +import java.util.Iterator; +import java.util.List; + import com.typesafe.config.ConfigException; final class Path { @@ -27,6 +30,25 @@ final class Path { } } + // append all the paths in the list together into one path + Path(List pathsToConcat) { + if (pathsToConcat.isEmpty()) + throw new ConfigException.BugOrBroken("empty path"); + + Iterator i = pathsToConcat.iterator(); + Path firstPath = i.next(); + this.first = firstPath.first; + + PathBuilder pb = new PathBuilder(); + if (firstPath.remainder != null) { + pb.appendPath(firstPath.remainder); + } + while (i.hasNext()) { + pb.appendPath(i.next()); + } + this.remainder = pb.result(); + } + String first() { return first; } @@ -35,6 +57,33 @@ final class Path { return remainder; } + Path prepend(Path toPrepend) { + PathBuilder pb = new PathBuilder(); + pb.appendPath(toPrepend); + pb.appendPath(this); + return pb.result(); + } + + int length() { + int count = 1; + Path p = remainder; + while (p != null) { + count += 1; + p = p.remainder; + } + return count; + } + + Path subPath(int removeFromFront) { + int count = removeFromFront; + Path p = this; + while (p != null && count > 0) { + count -= 1; + p = p.remainder; + } + return p; + } + @Override public boolean equals(Object other) { if (other instanceof Path) { diff --git a/src/main/java/com/typesafe/config/impl/PathBuilder.java b/src/main/java/com/typesafe/config/impl/PathBuilder.java index 550af117..3f3643e8 100644 --- a/src/main/java/com/typesafe/config/impl/PathBuilder.java +++ b/src/main/java/com/typesafe/config/impl/PathBuilder.java @@ -25,7 +25,25 @@ final class PathBuilder { keys.push(key); } + void appendPath(Path path) { + checkCanAppend(); + + String first = path.first(); + Path remainder = path.remainder(); + while (true) { + keys.push(first); + if (remainder != null) { + first = remainder.first(); + remainder = remainder.remainder(); + } else { + break; + } + } + } + Path result() { + // note: if keys is empty, we want to return null, which is a valid + // empty path if (result == null) { Path remainder = null; while (!keys.isEmpty()) { diff --git a/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index 54b463a3..f02c2402 100644 --- a/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -47,21 +47,16 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { return ResolveStatus.fromBoolean(resolved); } - @Override - SimpleConfigList resolveSubstitutions(SubstitutionResolver resolver, int depth, - boolean withFallbacks) { - if (resolved) - return this; - + private SimpleConfigList modify(Modifier modifier, + ResolveStatus newResolveStatus) { // lazy-create for optimization List changed = null; int i = 0; for (AbstractConfigValue v : value) { - AbstractConfigValue resolved = resolver.resolve(v, depth, - withFallbacks); + AbstractConfigValue modified = modifier.modifyChild(v); // lazy-create the new list if required - if (changed == null && resolved != v) { + if (changed == null && modified != v) { changed = new ArrayList(); for (int j = 0; j < i; ++j) { changed.add(value.get(j)); @@ -71,7 +66,7 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { // once the new list is created, all elements // have to go in it. if (changed != null) { - changed.add(resolved); + changed.add(modified); } i += 1; @@ -81,13 +76,38 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { if (changed.size() != value.size()) throw new ConfigException.BugOrBroken( "substituted list's size doesn't match"); - return new SimpleConfigList(origin(), changed, - ResolveStatus.RESOLVED); + return new SimpleConfigList(origin(), changed, newResolveStatus); } else { return this; } } + @Override + SimpleConfigList resolveSubstitutions(final SubstitutionResolver resolver, + final int depth, final boolean withFallbacks) { + if (resolved) + return this; + + return modify(new Modifier() { + @Override + public AbstractConfigValue modifyChild(AbstractConfigValue v) { + return resolver.resolve(v, depth, withFallbacks); + } + + }, ResolveStatus.RESOLVED); + } + + @Override + SimpleConfigList relativized(final Path prefix) { + return modify(new Modifier() { + @Override + public AbstractConfigValue modifyChild(AbstractConfigValue v) { + return v.relativized(prefix); + } + + }, resolveStatus()); + } + @Override protected boolean canEqual(Object other) { return other instanceof SimpleConfigList; diff --git a/src/test/resources/test01.conf b/src/test/resources/test01.conf index 54d94dda..98786ad1 100644 --- a/src/test/resources/test01.conf +++ b/src/test/resources/test01.conf @@ -62,5 +62,17 @@ "megsList" : [1M, 1024K, 1048576], "megAsNumber" : 1048576, "halfMeg" : 0.5M + }, + + "system" : { + "javaversion" : ${java.version}, + "userhome" : ${user.home}, + "home" : ${HOME}, + "pwd" : ${PWD}, + "shell" : ${SHELL}, + "lang" : ${LANG}, + "path" : ${PATH}, + "not_here" : ${NOT_HERE}, + "concatenated" : Your Java version is ${system.javaversion} and your user.home is ${system.userhome} } } diff --git a/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala b/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala index af77c5e7..687004fd 100644 --- a/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -284,4 +284,29 @@ class ConfigSubstitutionTest extends TestUtils { throw new Exception("None of the env vars we tried to use for testing were set") } } + + @Test + def fallbackToEnvWhenRelativized() { + import scala.collection.JavaConverters._ + + val values = new java.util.HashMap[String, AbstractConfigValue]() + + values.put("a", substEnvVarObject.relativized(new Path("a"))) + + val resolved = resolve(new SimpleConfigObject(fakeOrigin(), values)); + + var existed = 0 + for (k <- resolved.getObject("a").keySet().asScala) { + val e = System.getenv(k.toUpperCase()); + if (e != null) { + existed += 1 + assertEquals(e, resolved.getObject("a").getString(k)) + } else { + assertEquals(nullValue, resolved.getObject("a").get(k)) + } + } + if (existed == 0) { + throw new Exception("None of the env vars we tried to use for testing were set") + } + } } diff --git a/src/test/scala/com/typesafe/config/impl/ConfigTest.scala b/src/test/scala/com/typesafe/config/impl/ConfigTest.scala index fa150a3a..14111ac6 100644 --- a/src/test/scala/com/typesafe/config/impl/ConfigTest.scala +++ b/src/test/scala/com/typesafe/config/impl/ConfigTest.scala @@ -606,6 +606,20 @@ class ConfigTest extends TestUtils { conf.toString() } + @Test + def test01SystemFallbacks() { + val conf = Config.load("test01") + val jv = System.getProperty("java.version") + assertNotNull(jv) + assertEquals(jv, conf.getString("system.javaversion")) + val home = System.getenv("HOME") + if (home != null) { + assertEquals(home, conf.getString("system.home")) + } else { + assertEquals(nullValue, conf.get("system.home")) + } + } + @Test def test01LoadWithConfigConfig() { val conf = Config.load(new ConfigConfig("test01")) @@ -651,6 +665,26 @@ class ConfigTest extends TestUtils { assertEquals(57, conf.getInt("test02.a.b.c")) // equiv01/original.json was included (it has a slash in the name) assertEquals("a", conf.getString("equiv01.strings.a")) + + // Now check that substitutions still work + assertEquals(42, conf.getInt("test01.ints.fortyTwoAgain")) + assertEquals(Seq("a", "b", "c"), conf.getStringList("test01.arrays.ofString").asScala) + assertEquals(103, conf.getInt("test02.103_a")) + + // and system fallbacks still work + val jv = System.getProperty("java.version") + assertNotNull(jv) + assertEquals(jv, conf.getString("test01.system.javaversion")) + val home = System.getenv("HOME") + if (home != null) { + assertEquals(home, conf.getString("test01.system.home")) + } else { + assertEquals(nullValue, conf.get("test01.system.home")) + } + val concatenated = conf.getString("test01.system.concatenated") + assertTrue(concatenated.contains("Your Java version")) + assertTrue(concatenated.contains(jv)) + assertTrue(concatenated.contains(conf.getString("test01.system.userhome"))) } @Test diff --git a/src/test/scala/com/typesafe/config/impl/PathTest.scala b/src/test/scala/com/typesafe/config/impl/PathTest.scala index 6ed775de..0b16dced 100644 --- a/src/test/scala/com/typesafe/config/impl/PathTest.scala +++ b/src/test/scala/com/typesafe/config/impl/PathTest.scala @@ -2,6 +2,7 @@ package com.typesafe.config.impl import org.junit.Assert._ import org.junit._ +import scala.collection.JavaConverters._ class PathTest extends TestUtils { @@ -42,4 +43,23 @@ class PathTest extends TestUtils { assertEquals("\"foo.bar\"", path("foo.bar").render()) assertEquals("foo bar", path("foo bar").render()) } + + @Test + def pathFromPathList() { + assertEquals(path("foo"), new Path(List(path("foo")).asJava)) + assertEquals(path("foo", "bar", "baz", "boo"), new Path(List(path("foo", "bar"), + path("baz", "boo")).asJava)) + } + + @Test + def pathPrepend() { + assertEquals(path("foo", "bar"), path("bar").prepend(path("foo"))) + assertEquals(path("a", "b", "c", "d"), path("c", "d").prepend(path("a", "b"))) + } + + @Test + def pathLength() { + assertEquals(1, path("foo").length()) + assertEquals(2, path("foo", "bar").length()) + } }