From bda507b4742fad7399932b1ec4be92ae15cffd50 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 20 Jan 2012 18:34:55 -0500 Subject: [PATCH] Fix behavior of ${?} optional substitution overriding a field The broken behavior was: foo=10 foo=${?bar} where 'foo' should be 10 if bar is undefined, but instead ended up undefined. Now, foo will be 10 (which was the documented behavior). --- HOCON.md | 3 +- .../config/impl/AbstractConfigValue.java | 12 +++-- .../config/impl/ConfigSubstitution.java | 22 +++++++-- .../config/impl/ConfigSubstitutionTest.scala | 45 +++++++++++++++++++ 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/HOCON.md b/HOCON.md index ce0339bc..01b697fd 100644 --- a/HOCON.md +++ b/HOCON.md @@ -443,7 +443,8 @@ is invalid and should generate an error. If a substitution with the `${?foo}` syntax is undefined: - if it is the value of an object field then the field should not - be created. + be created. If the field would have overridden a previously-set + value for the same field, then the previous value remains. - if it is an array element then the element should not be added. - if it is part of a value concatenation then it should become an empty string. diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index e51f4c60..e78776ff 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -103,6 +103,13 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue { throw badMergeException(); } + protected AbstractConfigValue mergedWithNonObject(AbstractConfigValue fallback) { + // falling back to a non-object doesn't merge anything, and also + // prohibits merging any objects that we fall back to later. + // so we have to switch to ignoresFallbacks mode. + return newCopy(true /* ignoresFallbacks */, origin); + } + public AbstractConfigValue withOrigin(ConfigOrigin origin) { if (this.origin == origin) return this; @@ -130,10 +137,7 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue { return mergedWithObject((AbstractConfigObject) other); } } else { - // falling back to a non-object doesn't merge anything, and also - // prohibits merging any objects that we fall back to later. - // so we have to switch to ignoresFallbacks mode. - return newCopy(true /* ignoresFallbacks */, origin); + return mergedWithNonObject((AbstractConfigValue) other); } } } 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 f4441b81..6fce27a8 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -84,13 +84,10 @@ final class ConfigSubstitution extends AbstractConfigValue implements ((AbstractConfigValue) fallback).ignoresFallbacks()); } - @Override - protected AbstractConfigValue mergedWithObject(AbstractConfigObject fallback) { + protected AbstractConfigValue mergedLater(AbstractConfigValue fallback) { if (ignoresFallbacks) throw new ConfigException.BugOrBroken("should not be reached"); - // if we turn out to be an object, and the fallback also does, - // then a merge may be required; delay until we resolve. List newStack = new ArrayList(); newStack.add(this); newStack.add(fallback); @@ -98,6 +95,23 @@ final class ConfigSubstitution extends AbstractConfigValue implements fallback.ignoresFallbacks()); } + @Override + protected AbstractConfigValue mergedWithObject(AbstractConfigObject fallback) { + // if we turn out to be an object, and the fallback also does, + // then a merge may be required; delay until we resolve. + return mergedLater(fallback); + } + + @Override + protected AbstractConfigValue mergedWithNonObject(AbstractConfigValue fallback) { + // if the optional substitution ends up getting deleted (because it is + // not present), we'll have to use the fallback. So delay the merge. + if (pieces.size() == 1 && ((SubstitutionExpression) pieces.get(0)).optional()) + return mergedLater(fallback); + else + return super.mergedWithNonObject(fallback); + } + @Override public Collection unmergedValues() { return Collections.singleton(this); 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 73da3444..b6b03a73 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -425,4 +425,49 @@ class ConfigSubstitutionTest extends TestUtils { resolve(obj) } } + + @Test + def optionalOverrideNotProvided() { + val obj = parseObject("""{ a: 42, a : ${?NOT_HERE} }""") + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("a")) + } + + @Test + def optionalOverrideProvided() { + val obj = parseObject("""{ HERE : 43, a: 42, a : ${?HERE} }""") + val resolved = resolve(obj) + assertEquals(43, resolved.getInt("a")) + } + + @Test + def optionalOverrideOfObjectNotProvided() { + val obj = parseObject("""{ a: { b : 42 }, a : ${?NOT_HERE} }""") + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("a.b")) + } + + @Test + def optionalOverrideOfObjectProvided() { + val obj = parseObject("""{ HERE : 43, a: { b : 42 }, a : ${?HERE} }""") + val resolved = resolve(obj) + assertEquals(43, resolved.getInt("a")) + assertFalse(resolved.hasPath("a.b")) + } + + @Test + def optionalVanishesFromArray() { + import scala.collection.JavaConverters._ + val obj = parseObject("""{ a : [ 1, 2, 3, ${?NOT_HERE} ] }""") + val resolved = resolve(obj) + assertEquals(Seq(1, 2, 3), resolved.getIntList("a").asScala) + } + + @Test + def optionalUsedInArray() { + import scala.collection.JavaConverters._ + val obj = parseObject("""{ HERE: 4, a : [ 1, 2, 3, ${?HERE} ] }""") + val resolved = resolve(obj) + assertEquals(Seq(1, 2, 3, 4), resolved.getIntList("a").asScala) + } }