From 6ec2681055bd99381bcd954cfcda0c7ffbd0151c Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sun, 27 Nov 2011 11:59:22 -0500 Subject: [PATCH] missing, optional substitutions ${?foo} become undefined not null This is more useful behavior because it permits an optional override for example. --- README.md | 21 +++++++++++ .../config/impl/AbstractConfigObject.java | 9 ++++- .../config/impl/ConfigDelayedMerge.java | 10 +++--- .../config/impl/ConfigSubstitution.java | 10 ++---- .../config/impl/SimpleConfigList.java | 9 ++--- .../config/impl/SubstitutionResolver.java | 10 ++++-- .../src/test/resources/equiv04/original.json | 3 -- .../config/impl/ConfigSubstitutionTest.scala | 35 +++++++++++++++++-- .../typesafe/config/impl/PublicApiTest.scala | 4 +-- .../com/typesafe/config/impl/TestUtils.scala | 3 ++ 10 files changed, 85 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 85a48483..91de787e 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,27 @@ You can take advantage of this for "inheritance": Using `include` statements you could split this across multiple files, too. +#### Optional system or env variable overrides + +In default uses of the library, exact-match system properties +already override the corresponding config properties. However, +you can add your own overrides, or allow environment variables to +override, using the `${?foo}` substitution syntax. + + basedir = "/whatever/whatever" + basedir = ${?FORCED_BASEDIR} + +Here, the override field `basedir = ${?FORCED_BASEDIR}` simply +vanishes if there's no value for `FORCED_BASEDIR`, but if you set +an environment variable `FORCED_BASEDIR` for example, it would be +used. + +Object fields and array elements with a `${?foo}` substitution +value just disappear if the substitution is not found. + + // this array could have one or two elements + path = [ "a", ${?OPTIONAL_A} ] + ## Future Directions Here are some features that might be nice to add. diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index 807a64a8..70c93128 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -200,6 +200,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Map changes = null; for (String k : keySet()) { AbstractConfigValue v = peek(k); + // "modified" may be null, which means remove the child; + // to do that we put null in the "changes" map. AbstractConfigValue modified = modifier.modifyChild(v); if (modified != v) { if (changes == null) @@ -213,7 +215,12 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Map modified = new HashMap(); for (String k : keySet()) { if (changes.containsKey(k)) { - modified.put(k, changes.get(k)); + AbstractConfigValue newValue = changes.get(k); + if (newValue != null) { + modified.put(k, newValue); + } else { + // remove this child; don't put it in the new map. + } } else { modified.put(k, peek(k)); } diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java index 4c7d66da..b916d9a0 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -77,10 +77,12 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements AbstractConfigValue merged = null; for (AbstractConfigValue v : stack) { AbstractConfigValue resolved = resolver.resolve(v, depth, options); - if (merged == null) - merged = resolved; - else - merged = merged.withFallback(resolved); + if (resolved != null) { + if (merged == null) + merged = resolved; + else + merged = merged.withFallback(resolved); + } } return merged; diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java index e6f2bc6b..1b274086 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -191,14 +191,8 @@ final class ConfigSubstitution extends AbstractConfigValue implements "ConfigSubstitution should never contain a single String piece"); SubstitutionExpression exp = (SubstitutionExpression) pieces.get(0); ConfigValue v = resolve(resolver, exp, depth, options); - if (v == null) { - if (exp.optional()) { - // FIXME want to delete the field or array element here - // instead of this - v = new ConfigNull(origin()); - } else { - throw new ConfigException.UnresolvedSubstitution(origin(), exp.toString()); - } + if (v == null && !exp.optional()) { + throw new ConfigException.UnresolvedSubstitution(origin(), exp.toString()); } return v; } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index 76d99637..cdf1a179 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -9,7 +9,6 @@ import java.util.Iterator; import java.util.List; import java.util.ListIterator; -import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigList; import com.typesafe.config.ConfigOrigin; import com.typesafe.config.ConfigResolveOptions; @@ -68,8 +67,9 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { } // once the new list is created, all elements - // have to go in it. - if (changed != null) { + // have to go in it. if modifyChild returned + // null, we drop that element. + if (changed != null && modified != null) { changed.add(modified); } @@ -77,9 +77,6 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { } if (changed != null) { - if (changed.size() != value.size()) - throw new ConfigException.BugOrBroken( - "substituted list's size doesn't match"); return new SimpleConfigList(origin(), changed, newResolveStatus); } else { return this; diff --git a/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java b/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java index 7f77570d..65a2b309 100644 --- a/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java +++ b/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java @@ -16,6 +16,8 @@ import com.typesafe.config.ConfigResolveOptions; */ final class SubstitutionResolver { final private AbstractConfigObject root; + // note that we can resolve things to undefined (represented as Java null, + // rather than ConfigNull) so this map can have null values. final private Map memos; SubstitutionResolver(AbstractConfigObject root) { @@ -31,9 +33,11 @@ final class SubstitutionResolver { } else { AbstractConfigValue resolved = original.resolveSubstitutions(this, depth, options); - if (resolved.resolveStatus() != ResolveStatus.RESOLVED) - throw new ConfigException.BugOrBroken( - "resolveSubstitutions() did not give us a resolved object"); + if (resolved != null) { + if (resolved.resolveStatus() != ResolveStatus.RESOLVED) + throw new ConfigException.BugOrBroken( + "resolveSubstitutions() did not give us a resolved object"); + } memos.put(original, resolved); return resolved; } diff --git a/config/src/test/resources/equiv04/original.json b/config/src/test/resources/equiv04/original.json index fbdfcd30..bfd870e4 100644 --- a/config/src/test/resources/equiv04/original.json +++ b/config/src/test/resources/equiv04/original.json @@ -1,6 +1,3 @@ { - "a" : null, - "b" : null, - "c" : null } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala index 772f17a3..984a3679 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -127,6 +127,11 @@ class ConfigSubstitutionTest extends TestUtils { val v = resolveWithoutFallbacks(s, simpleObject) // absent object becomes empty string assertEquals(stringValue("start<>end"), v) + + intercept[ConfigException.UnresolvedSubstitution] { + val s2 = substInString("bar.missing", false /* optional */ ) + resolveWithoutFallbacks(s2, simpleObject) + } } @Test @@ -150,6 +155,32 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(stringValue("start<3.14>end"), v) } + @Test + def missingInArray() { + import scala.collection.JavaConverters._ + + val obj = parseObject(""" + a : [ ${?missing}, ${?also.missing} ] +""") + + val resolved = resolve(obj) + + assertEquals(Seq(), resolved.getList("a").asScala) + } + + @Test + def missingInObject() { + import scala.collection.JavaConverters._ + + val obj = parseObject(""" + a : ${?missing}, b : ${?also.missing}, c : ${?b}, d : ${?c} +""") + + val resolved = resolve(obj) + + assertTrue(resolved.isEmpty()) + } + private val substChainObject = { parseObject(""" { @@ -298,7 +329,7 @@ class ConfigSubstitutionTest extends TestUtils { existed += 1 assertEquals(e, resolved.getString(k)) } else { - assertEquals(nullValue, resolved.root.get(k)) + assertNull(resolved.root.get(k)) } } if (existed == 0) { @@ -345,7 +376,7 @@ class ConfigSubstitutionTest extends TestUtils { existed += 1 assertEquals(e, resolved.getConfig("a").getString(k)) } else { - assertEquals(nullValue, resolved.getObject("a").get(k)) + assertNull(resolved.getObject("a").get(k)) } } if (existed == 0) { 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 4e04694f..c14e717b 100644 --- a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala @@ -37,10 +37,10 @@ class PublicApiTest extends TestUtils { val conf = ConfigFactory.load("test01", ConfigParseOptions.defaults(), ConfigResolveOptions.noSystem()) - intercept[ConfigException.Null] { + intercept[ConfigException.Missing] { conf.getString("system.home") } - intercept[ConfigException.Null] { + intercept[ConfigException.Missing] { conf.getString("system.javaversion") } } diff --git a/config/src/test/scala/com/typesafe/config/impl/TestUtils.scala b/config/src/test/scala/com/typesafe/config/impl/TestUtils.scala index 8550b982..27eb3082 100644 --- a/config/src/test/scala/com/typesafe/config/impl/TestUtils.scala +++ b/config/src/test/scala/com/typesafe/config/impl/TestUtils.scala @@ -149,6 +149,8 @@ abstract trait TestUtils { "[$]", // '$' by itself "[$ ]", // '$' by itself with spaces after "[${}]", // empty substitution (no path) + "[${?}]", // no path with ? substitution + ParseTest(false, true, "[${ ?foo}]"), // space before ? not allowed """{ "a" : [1,2], "b" : y${a}z }""", // trying to interpolate an array in a string """{ "a" : { "c" : 2 }, "b" : y${a}z }""", // trying to interpolate an object in a string """{ "a" : ${a} }""", // simple cycle @@ -249,6 +251,7 @@ abstract trait TestUtils { "{ include \"foo\", \"a\" : \"b\" }", // valid include followed by comma and field "{ foo include : 42 }", // valid to have a key not starting with include "[ ${foo} ]", + "[ ${?foo} ]", "[ ${\"foo\"} ]", "[ ${foo.bar} ]", "[ abc xyz ${foo.bar} qrs tuv ]", // value concatenation