From d70040424249cb20798544a35cd98fd5b8ab2638 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Thu, 17 Nov 2011 22:23:06 -0500 Subject: [PATCH] fix AbstractConfigValue.withFallbacks and clean up ConfigDelayedMerge withFallback Had previously fixed AbstractConfigObject.merge() in the same way. For this patch, factor out the code from AbstractConfigObject and use it everywhere. Add some tests for merges with substitutions involved. Also write some docs for ConfigMergeable explaining the easy way to do it wrong. --- .../com/typesafe/config/ConfigMergeable.java | 33 +++++- .../config/impl/AbstractConfigObject.java | 26 ----- .../config/impl/AbstractConfigValue.java | 6 +- .../config/impl/ConfigDelayedMerge.java | 27 +---- .../com/typesafe/config/impl/ConfigImpl.java | 29 +++++ .../com/typesafe/config/impl/ConfigTest.scala | 104 ++++++++++++++---- 6 files changed, 149 insertions(+), 76 deletions(-) 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) }