diff --git a/src/main/java/com/typesafe/config/ConfigMergeable.java b/src/main/java/com/typesafe/config/ConfigMergeable.java index 316fa857..c1b0b6fc 100644 --- a/src/main/java/com/typesafe/config/ConfigMergeable.java +++ b/src/main/java/com/typesafe/config/ConfigMergeable.java @@ -7,7 +7,7 @@ package com.typesafe.config; public interface ConfigMergeable { /** * Converts the mergeable to a ConfigValue to be merged. - * + * * @return */ ConfigValue toValue(); @@ -19,6 +19,34 @@ public interface ConfigMergeable { * fallback keys into themselves). All other values just return the original * value, since they automatically override any fallback. * + * The semantics of merging are described in + * https://github.com/havocp/config/blob/master/HOCON.md + * + * Prefer withFallbacks(), listing all your fallbacks at once, + * over this method. + * + * When using this method, there is an easy way to write a wrong + * loop. Even if you don't loop, it's easy to do the equivalent wrong + * thing. + * + * + * // WRONG + * for (ConfigMergeable fallback : fallbacks) { + * // DO NOT DO THIS + * result = result.withFallback(fallback); + * } + * + * + * This is wrong because when result is an object and + * fallback is a non-object, + * result.withFallback(fallback) returns an object. Then if + * there are more objects, they are merged into that object. But the correct + * semantics are that a non-object will block merging any more objects later + * in the list. To get it right, you need to iterate backward. Simplest + * solution: prefer withFallbacks() which is harder to get + * wrong, and merge all your fallbacks in one call to + * withFallbacks(). + * * @param other * an object whose keys should be used if the keys are not * present in this one @@ -29,7 +57,8 @@ public interface ConfigMergeable { /** * Convenience method just calls withFallback() on each of the values; - * earlier values in the list win over later ones. + * earlier values in the list win over later ones. The semantics of merging + * are described in https://github.com/havocp/config/blob/master/HOCON.md * * @param fallbacks * @return a version of the object with the requested fallbacks merged in diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index 56b3f746..6e4ff47c 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -7,7 +7,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.ListIterator; import java.util.Map; import java.util.Set; @@ -194,31 +193,6 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements return mergeOrigins(Arrays.asList(stack)); } - /** - * Stack should be from overrides to fallbacks (earlier items win). Objects - * have their keys combined into a new object, while other kinds of value - * are just first-one-wins. - */ - static AbstractConfigObject merge(List stack) { - if (stack.isEmpty()) { - return SimpleConfigObject.empty(); - } else if (stack.size() == 1) { - return stack.get(0); - } else { - // to be consistent with the semantics of duplicate keys - // in the same file, we have to go backward like this. - // importantly, a primitive value always permanently - // hides a previous object value. - ListIterator i = stack.listIterator(stack - .size()); - AbstractConfigObject merged = i.previous(); - while (i.hasPrevious()) { - merged = i.previous().withFallback(merged); - } - return merged; - } - } - private AbstractConfigObject modify(Modifier modifier, ResolveStatus newResolveStatus) { Map changes = null; diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index 7474f5e8..c4e5c3bf 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -82,11 +82,7 @@ abstract class AbstractConfigValue implements ConfigValue { // note: this is a no-op unless the subclass overrides withFallback(). // But we need to do this because subclass withFallback() may not // just "return this" - AbstractConfigValue merged = this; - for (ConfigMergeable f : fallbacks) { - merged = merged.withFallback(f.toValue()); - } - return merged; + return ConfigImpl.merge(AbstractConfigValue.class, this, fallbacks); } protected boolean canEqual(Object other) { diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java index df576385..3215958d 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -58,32 +58,17 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements int depth, ConfigResolveOptions options) { // to resolve substitutions, we need to recursively resolve // the stack of stuff to merge, and then merge the stack. - List toMerge = new ArrayList(); + List toMerge = new ArrayList(); for (AbstractConfigValue v : stack) { AbstractConfigValue resolved = resolver.resolve(v, depth, options); - - if (resolved instanceof AbstractConfigObject) { - toMerge.add((AbstractConfigObject) resolved); - } else { - if (toMerge.isEmpty()) { - // done, we'll ignore any objects anyway since we - // now have a non-object value. There is a semantic - // effect to this optimization though: we won't detect - // any cycles or other errors in the objects we are - // not resolving. In other cases (without substitutions - // involved) we might detect those errors. - return resolved; - } else { - // look for more objects to merge, since once we have - // an object we merge all objects even if there are - // intervening non-objects. - continue; - } - } + toMerge.add(resolved); } - return AbstractConfigObject.merge(toMerge); + // we shouldn't have a delayed merge object with an empty stack, so + // it should be safe to ignore the toMerge.isEmpty case. + return ConfigImpl.merge(AbstractConfigValue.class, toMerge.get(0), + toMerge.subList(1, toMerge.size())); } @Override diff --git a/src/main/java/com/typesafe/config/impl/ConfigImpl.java b/src/main/java/com/typesafe/config/impl/ConfigImpl.java index d7e1ef0c..d8606e75 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -1,16 +1,19 @@ package com.typesafe.config.impl; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.ListIterator; import java.util.Map; import com.typesafe.config.Config; import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigIncludeContext; import com.typesafe.config.ConfigIncluder; +import com.typesafe.config.ConfigMergeable; import com.typesafe.config.ConfigObject; import com.typesafe.config.ConfigOrigin; import com.typesafe.config.ConfigParseOptions; @@ -22,6 +25,32 @@ import com.typesafe.config.ConfigValue; /** This is public but is only supposed to be used by the "config" package */ public class ConfigImpl { + static T merge(Class klass, T first, + ConfigMergeable... others) { + List stack = Arrays.asList(others); + return merge(klass, first, stack); + } + + static T merge(Class klass, T first, + List stack) { + if (stack.isEmpty()) { + return first; + } else { + // to be consistent with the semantics of duplicate keys + // in the same file, we have to go backward like this. + // importantly, a primitive value always permanently + // hides a previous object value. + ListIterator i = stack + .listIterator(stack.size()); + ConfigMergeable merged = i.previous(); + while (i.hasPrevious()) { + merged = i.previous().withFallback(merged); + } + merged = first.withFallback(merged); + return klass.cast(merged); + } + } + private interface NameSource { ConfigParseable nameToParseable(String name); } diff --git a/src/test/scala/com/typesafe/config/impl/ConfigTest.scala b/src/test/scala/com/typesafe/config/impl/ConfigTest.scala index 96f3eb8f..a67893fb 100644 --- a/src/test/scala/com/typesafe/config/impl/ConfigTest.scala +++ b/src/test/scala/com/typesafe/config/impl/ConfigTest.scala @@ -15,8 +15,31 @@ import com.typesafe.config.ConfigFactory class ConfigTest extends TestUtils { + private def resolveNoSystem(v: AbstractConfigValue, root: AbstractConfigObject) = { + SubstitutionResolver.resolve(v, root, ConfigResolveOptions.noSystem()) + } + + private def resolveNoSystem(v: SimpleConfig, root: SimpleConfig) = { + SubstitutionResolver.resolve(v.toObject, root.toObject, + ConfigResolveOptions.noSystem()).asInstanceOf[AbstractConfigObject].toConfig + } + + def mergeUnresolved(toMerge: AbstractConfigObject*) = { + if (toMerge.isEmpty) { + SimpleConfigObject.empty() + } else { + val obj = toMerge(0).withFallbacks(toMerge.slice(1, toMerge.size): _*) + obj match { + case x: AbstractConfigObject => x + } + } + } + def merge(toMerge: AbstractConfigObject*) = { - AbstractConfigObject.merge(toMerge.toList.asJava) + val obj = mergeUnresolved(toMerge: _*) + resolveNoSystem(obj, obj) match { + case x: AbstractConfigObject => x + } } // In many cases, we expect merging to be associative. It is @@ -25,7 +48,7 @@ class ConfigTest extends TestUtils { // in that case, if an association starts with the non-object // value, then the value after the non-object value gets lost. private def associativeMerge(allObjects: Seq[AbstractConfigObject])(assertions: SimpleConfig => Unit) { - def m(toMerge: AbstractConfigObject*) = merge(toMerge: _*) + def m(toMerge: AbstractConfigObject*) = mergeUnresolved(toMerge: _*) def makeTrees(objects: Seq[AbstractConfigObject]): Iterator[AbstractConfigObject] = { objects.length match { @@ -187,6 +210,23 @@ class ConfigTest extends TestUtils { assertEquals(1, merged2.getObject("a").size) } + @Test + def mergeOverrideObjectAndSubstitution() { + val obj1 = parseObject("""{ "a" : 1 }""") + val obj2 = parseObject("""{ "a" : { "b" : ${c} }, "c" : 42 }""") + val merged = merge(obj1, obj2).toConfig + + assertEquals(1, merged.getInt("a")) + assertEquals(2, merged.toObject.size) + + val merged2 = merge(obj2, obj1).toConfig + + assertEquals(42, merged2.getConfig("a").getInt("b")) + assertEquals(42, merged2.getInt("a.b")) + assertEquals(2, merged2.toObject.size) + assertEquals(1, merged2.getObject("a").size) + } + @Test def mergeObjectThenPrimitiveThenObject() { // the semantic here is that the primitive blocks the @@ -210,6 +250,29 @@ class ConfigTest extends TestUtils { assertEquals(2, merged2.getObject("a").size()) } + @Test + def mergeObjectThenSubstitutionThenObject() { + // the semantic here is that the primitive blocks the + // object that occurs at lower priority. This is consistent + // with duplicate keys in the same file. + val obj1 = parseObject("""{ "a" : { "b" : ${f} } }""") + val obj2 = parseObject("""{ "a" : 2 }""") + val obj3 = parseObject("""{ "a" : { "b" : ${d}, "c" : ${e} }, "d" : 43, "e" : 44, "f" : 42 }""") + + val merged = merge(obj1, obj2, obj3).toConfig + + assertEquals(42, merged.getInt("a.b")) + assertEquals(4, merged.toObject.size) + assertEquals(1, merged.getObject("a").size()) + + val merged2 = merge(obj3, obj2, obj1).toConfig + + assertEquals(43, merged2.getInt("a.b")) + assertEquals(44, merged2.getInt("a.c")) + assertEquals(4, merged2.toObject.size) + assertEquals(2, merged2.getObject("a").size()) + } + @Test def mergePrimitiveThenObjectThenPrimitive() { // the primitive should override the object @@ -223,13 +286,19 @@ class ConfigTest extends TestUtils { } } - private def resolveNoSystem(v: AbstractConfigValue, root: AbstractConfigObject) = { - SubstitutionResolver.resolve(v, root, ConfigResolveOptions.noSystem()) - } + @Test + def mergeSubstitutionThenObjectThenSubstitution() { + // the substitution should override the object + val obj1 = parseObject("""{ "a" : ${b}, "b" : 1 }""") + val obj2 = parseObject("""{ "a" : { "b" : 42 } }""") + val obj3 = parseObject("""{ "a" : ${c}, "c" : 2 }""") - private def resolveNoSystem(v: SimpleConfig, root: SimpleConfig) = { - SubstitutionResolver.resolve(v.toObject, root.toObject, - ConfigResolveOptions.noSystem()).asInstanceOf[AbstractConfigObject].toConfig + associativeMerge(Seq(obj1, obj2, obj3)) { merged => + val resolved = resolveNoSystem(merged, merged) + + assertEquals(1, resolved.getInt("a")) + assertEquals(3, resolved.toObject.size) + } } @Test @@ -237,10 +306,7 @@ class ConfigTest extends TestUtils { val obj1 = parseObject("""{ "a" : { "x" : 1, "z" : 4 }, "c" : ${a} }""") val obj2 = parseObject("""{ "b" : { "y" : 2, "z" : 5 }, "c" : ${b} }""") - val merged = merge(obj1, obj2) - val resolved = (resolveNoSystem(merged, merged) match { - case x: ConfigObject => x - }).toConfig + val resolved = merge(obj1, obj2).toConfig assertEquals(3, resolved.getObject("c").size()) assertEquals(1, resolved.getInt("c.x")) @@ -253,19 +319,13 @@ class ConfigTest extends TestUtils { val obj1 = parseObject("""{ "a" : { "x" : 1, "z" : 4 }, "c" : { "z" : 42 } }""") val obj2 = parseObject("""{ "b" : { "y" : 2, "z" : 5 }, "c" : ${b} }""") - val merged = merge(obj1, obj2) - val resolved = (resolveNoSystem(merged, merged) match { - case x: ConfigObject => x - }).toConfig + val resolved = merge(obj1, obj2).toConfig assertEquals(2, resolved.getObject("c").size()) assertEquals(2, resolved.getInt("c.y")) assertEquals(42, resolved.getInt("c.z")) - val merged2 = merge(obj2, obj1) - val resolved2 = (resolveNoSystem(merged2, merged2) match { - case x: ConfigObject => x - }).toConfig + val resolved2 = merge(obj2, obj1).toConfig assertEquals(2, resolved2.getObject("c").size()) assertEquals(2, resolved2.getInt("c.y")) @@ -293,7 +353,7 @@ class ConfigTest extends TestUtils { assertTrue(e.getMessage().contains("cycle")) val fixUpCycle = parseObject(""" { "a" : { "b" : { "c" : 57 } } } """) - val merged = merge(fixUpCycle, cycleObject) + val merged = mergeUnresolved(fixUpCycle, cycleObject) val v = resolveNoSystem(subst("foo"), merged) assertEquals(intValue(57), v); } @@ -309,7 +369,7 @@ class ConfigTest extends TestUtils { assertTrue(e.getMessage().contains("cycle")) val fixUpCycle = parseObject(""" { "a" : { "b" : { "c" : { "q" : "u" } } } } """) - val merged = merge(fixUpCycle, cycleObject) + val merged = mergeUnresolved(fixUpCycle, cycleObject) val e2 = intercept[ConfigException.BadValue] { val v = resolveNoSystem(subst("foo"), merged) }