From 4b9e5ec325ca08721675011dad8056bfa251cc88 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 18 Nov 2011 14:47:36 -0500 Subject: [PATCH] Fix withFallback() to avoid the mess where it has to be called from the bottom up. Make ConfigObject have an includeFallbacks flag for whether it has "ended" and will no longer merge anything. --- .../com/typesafe/config/ConfigMergeable.java | 35 +++++-------------- .../config/impl/AbstractConfigObject.java | 27 ++++++++------ .../config/impl/AbstractConfigValue.java | 7 ++++ .../config/impl/ConfigDelayedMerge.java | 25 ++++++++++--- .../config/impl/ConfigDelayedMergeObject.java | 34 +++++++++++++----- .../com/typesafe/config/impl/ConfigImpl.java | 22 +++--------- .../config/impl/ConfigSubstitution.java | 16 +++++---- .../config/impl/PropertiesParser.java | 5 +-- .../config/impl/SimpleConfigObject.java | 20 +++++++---- .../config/impl/ConfigValueTest.scala | 3 +- 10 files changed, 112 insertions(+), 82 deletions(-) diff --git a/src/main/java/com/typesafe/config/ConfigMergeable.java b/src/main/java/com/typesafe/config/ConfigMergeable.java index 9c3b30bb..13fa336a 100644 --- a/src/main/java/com/typesafe/config/ConfigMergeable.java +++ b/src/main/java/com/typesafe/config/ConfigMergeable.java @@ -21,35 +21,16 @@ public interface ConfigMergeable { * Config instances do anything in this method (they need to merge the * 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(). - * + * + * Note that objects do not merge "across" non-objects; if you do + * object.withFallback(nonObject).withFallback(otherObject), + * then otherObject will simply be ignored. This is an + * intentional part of how merging works. Both non-objects, and any object + * which has fallen back to a non-object, block subsequent fallbacks. + * * @param other * an object whose keys should be used if the keys are not * present in this one diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index 8b0f1602..becdc73a 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -102,7 +102,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements return ConfigValueType.OBJECT; } - protected abstract AbstractConfigObject newCopy(ResolveStatus status); + protected abstract AbstractConfigObject newCopy(ResolveStatus status, + boolean ignoresFallbacks); @Override public AbstractConfigObject withFallbacks(ConfigMergeable... others) { @@ -113,7 +114,9 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements public AbstractConfigObject withFallback(ConfigMergeable mergeable) { ConfigValue other = mergeable.toValue(); - if (other instanceof Unmergeable) { + if (ignoresFallbacks()) { + return this; + } else if (other instanceof Unmergeable) { List stack = new ArrayList(); stack.add(this); stack.addAll(((Unmergeable) other).unmergedValues()); @@ -143,12 +146,14 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements allResolved = false; } return new SimpleConfigObject(mergeOrigins(this, fallback), - merged, ResolveStatus.fromBoolean(allResolved)); + merged, ResolveStatus.fromBoolean(allResolved), + ignoresFallbacks()); } } else { - // falling back to a non-object has no effect, we just override - // primitive values. - return this; + // 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(resolveStatus(), true /* ignoresFallbacks */); } } @@ -209,7 +214,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements } } if (changes == null) { - return newCopy(newResolveStatus); + return newCopy(newResolveStatus, ignoresFallbacks()); } else { Map modified = new HashMap(); for (String k : keySet()) { @@ -219,8 +224,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements modified.put(k, peek(k)); } } - return new SimpleConfigObject(origin(), modified, - newResolveStatus); + return new SimpleConfigObject(origin(), modified, newResolveStatus, + ignoresFallbacks()); } } @@ -314,7 +319,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements @Override public boolean equals(Object other) { - // note that "origin" is deliberately NOT part of equality + // note that "origin" is deliberately NOT part of equality. + // neither are other "extras" like ignoresFallbacks or resolve status. if (other instanceof ConfigObject) { // optimization to avoid unwrapped() for two ConfigObject, // which is what AbstractConfigValue does. @@ -327,6 +333,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements @Override public int hashCode() { // note that "origin" is deliberately NOT part of equality + // neither are other "extras" like ignoresFallbacks or resolve status. return mapHash(this); } diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index 63cb0d57..b05adf4a 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -75,6 +75,13 @@ abstract class AbstractConfigValue implements ConfigValue { return this; } + // this is virtualized rather than a field because only some subclasses + // really need to store the boolean, and they may be able to pack it + // with another boolean to save space. + protected boolean ignoresFallbacks() { + return true; + } + @Override public AbstractConfigValue withFallback(ConfigMergeable 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 0a8a7055..3b93b56f 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -27,16 +27,23 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements // earlier items in the stack win final private List stack; + final private boolean ignoresFallbacks; - ConfigDelayedMerge(ConfigOrigin origin, List stack) { + private ConfigDelayedMerge(ConfigOrigin origin, + List stack, boolean ignoresFallbacks) { super(origin); this.stack = stack; + this.ignoresFallbacks = ignoresFallbacks; if (stack.isEmpty()) throw new ConfigException.BugOrBroken( "creating empty delayed merge value"); } + ConfigDelayedMerge(ConfigOrigin origin, List stack) { + this(origin, stack, false /* ignoresFallbacks */); + } + @Override public ConfigValueType valueType() { throw new ConfigException.NotResolved( @@ -85,14 +92,21 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements for (AbstractConfigValue o : stack) { newStack.add(o.relativized(prefix)); } - return new ConfigDelayedMerge(origin(), newStack); + return new ConfigDelayedMerge(origin(), newStack, ignoresFallbacks); + } + + @Override + protected boolean ignoresFallbacks() { + return ignoresFallbacks; } @Override public AbstractConfigValue withFallback(ConfigMergeable mergeable) { ConfigValue other = mergeable.toValue(); - if (other instanceof AbstractConfigObject + if (ignoresFallbacks) { + return this; + } else if (other instanceof AbstractConfigObject || other instanceof Unmergeable) { // if we turn out to be an object, and the fallback also does, // then a merge may be required; delay until we resolve. @@ -103,11 +117,12 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements else newStack.add((AbstractConfigValue) other); return new ConfigDelayedMerge( - AbstractConfigObject.mergeOrigins(newStack), newStack); + AbstractConfigObject.mergeOrigins(newStack), newStack, + ignoresFallbacks); } else { // if the other is not an object, there won't be anything // to merge with, so we are it even if we are an object. - return this; + return new ConfigDelayedMerge(origin(), stack, true /* ignoresFallbacks */); } } diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java index c0a15f4d..5c4e8ead 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -21,11 +21,19 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements Unmergeable { final private List stack; + final private boolean ignoresFallbacks; ConfigDelayedMergeObject(ConfigOrigin origin, List stack) { + this(origin, stack, false /* ignoresFallbacks */); + } + + private ConfigDelayedMergeObject(ConfigOrigin origin, + List stack, boolean ignoresFallbacks) { super(origin); this.stack = stack; + this.ignoresFallbacks = ignoresFallbacks; + if (stack.isEmpty()) throw new ConfigException.BugOrBroken( "creating empty delayed merge object"); @@ -35,11 +43,12 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements } @Override - public ConfigDelayedMergeObject newCopy(ResolveStatus status) { + public ConfigDelayedMergeObject newCopy(ResolveStatus status, + boolean ignoresFallbacks) { if (status != resolveStatus()) throw new ConfigException.BugOrBroken( "attempt to create resolved ConfigDelayedMergeObject"); - return new ConfigDelayedMergeObject(origin(), stack); + return new ConfigDelayedMergeObject(origin(), stack, ignoresFallbacks); } @Override @@ -67,14 +76,22 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements for (AbstractConfigValue o : stack) { newStack.add(o.relativized(prefix)); } - return new ConfigDelayedMergeObject(origin(), newStack); + return new ConfigDelayedMergeObject(origin(), newStack, + ignoresFallbacks); + } + + @Override + protected boolean ignoresFallbacks() { + return ignoresFallbacks; } @Override public ConfigDelayedMergeObject withFallback(ConfigMergeable mergeable) { ConfigValue other = mergeable.toValue(); - if (other instanceof AbstractConfigObject + if (ignoresFallbacks) { + return this; + } else if (other instanceof AbstractConfigObject || other instanceof Unmergeable) { // since we are an object, and the fallback could be, // then a merge may be required; delay until we resolve. @@ -85,12 +102,13 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements else newStack.add((AbstractConfigValue) other); return new ConfigDelayedMergeObject( - AbstractConfigObject.mergeOrigins(newStack), - newStack); + AbstractConfigObject.mergeOrigins(newStack), newStack, + ignoresFallbacks); } else { // if the other is not an object, there won't be anything - // to merge with. - return this; + // to merge with but we do need to ignore any fallbacks + // that occur after it. + return newCopy(resolveStatus(), true /* ignoresFallbacks */); } } diff --git a/src/main/java/com/typesafe/config/impl/ConfigImpl.java b/src/main/java/com/typesafe/config/impl/ConfigImpl.java index e9fff371..a8ee038f 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -10,7 +10,6 @@ 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; @@ -37,22 +36,11 @@ public class ConfigImpl { 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); + ConfigMergeable merged = first; + for (ConfigMergeable fallback : stack) { + merged = merged.withFallback(fallback); } + return klass.cast(merged); } private interface NameSource { @@ -428,7 +416,7 @@ public class ConfigImpl { new SimpleConfigOrigin("env var " + key), entry.getValue())); } return new SimpleConfigObject(new SimpleConfigOrigin("env variables"), - m, ResolveStatus.RESOLVED); + m, ResolveStatus.RESOLVED, false /* ignoresFallbacks */); } /** For use ONLY by library internals, DO NOT TOUCH not guaranteed ABI */ diff --git a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java index 7f294d66..87492e94 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -28,17 +28,19 @@ final class ConfigSubstitution extends AbstractConfigValue implements // than one piece everything is stringified and concatenated final private List pieces; // the length of any prefixes added with relativized() - final int prefixLength; + final private int prefixLength; + final private boolean ignoresFallbacks; ConfigSubstitution(ConfigOrigin origin, List pieces) { - this(origin, pieces, 0); + this(origin, pieces, 0, false); } private ConfigSubstitution(ConfigOrigin origin, List pieces, - int prefixLength) { + int prefixLength, boolean ignoresFallbacks) { super(origin); this.pieces = pieces; this.prefixLength = prefixLength; + this.ignoresFallbacks = ignoresFallbacks; } @Override @@ -58,7 +60,9 @@ final class ConfigSubstitution extends AbstractConfigValue implements public AbstractConfigValue withFallback(ConfigMergeable mergeable) { ConfigValue other = mergeable.toValue(); - if (other instanceof AbstractConfigObject + if (ignoresFallbacks) { + return this; + } else if (other instanceof AbstractConfigObject || other instanceof Unmergeable) { // if we turn out to be an object, and the fallback also does, // then a merge may be required; delay until we resolve. @@ -73,7 +77,7 @@ final class ConfigSubstitution extends AbstractConfigValue implements } else { // if the other is not an object, there won't be anything // to merge with, so we are it even if we are an object. - return this; + return new ConfigSubstitution(origin(), pieces, prefixLength, true /* ignoresFallbacks */); } } @@ -201,7 +205,7 @@ final class ConfigSubstitution extends AbstractConfigValue implements } } return new ConfigSubstitution(origin(), newPieces, prefixLength - + prefix.length()); + + prefix.length(), ignoresFallbacks); } @Override diff --git a/src/main/java/com/typesafe/config/impl/PropertiesParser.java b/src/main/java/com/typesafe/config/impl/PropertiesParser.java index 5dd36e4b..bd822e65 100644 --- a/src/main/java/com/typesafe/config/impl/PropertiesParser.java +++ b/src/main/java/com/typesafe/config/impl/PropertiesParser.java @@ -180,11 +180,12 @@ final class PropertiesParser { .get(parentPath) : root; AbstractConfigObject o = new SimpleConfigObject(origin, scope, - ResolveStatus.RESOLVED); + ResolveStatus.RESOLVED, false /* ignoresFallbacks */); parent.put(scopePath.last(), o); } // return root config object - return new SimpleConfigObject(origin, root, ResolveStatus.RESOLVED); + return new SimpleConfigObject(origin, root, ResolveStatus.RESOLVED, + false /* ignoresFallbacks */); } } diff --git a/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java b/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java index 23fadc06..3534fca0 100644 --- a/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -20,21 +20,23 @@ final class SimpleConfigObject extends AbstractConfigObject { // this map should never be modified - assume immutable final private Map value; final private boolean resolved; + final private boolean ignoresFallbacks; SimpleConfigObject(ConfigOrigin origin, - Map value, ResolveStatus status) { + Map value, ResolveStatus status, + boolean ignoresFallbacks) { super(origin); if (value == null) throw new ConfigException.BugOrBroken( "creating config object with null map"); this.value = value; this.resolved = status == ResolveStatus.RESOLVED; + this.ignoresFallbacks = ignoresFallbacks; } SimpleConfigObject(ConfigOrigin origin, Map value) { - this(origin, value, ResolveStatus.fromValues(value - .values())); + this(origin, value, ResolveStatus.fromValues(value.values()), false /* ignoresFallbacks */); } @Override @@ -43,9 +45,10 @@ final class SimpleConfigObject extends AbstractConfigObject { } @Override - public SimpleConfigObject newCopy(ResolveStatus newStatus) { - return new SimpleConfigObject(origin(), value, - newStatus); + public SimpleConfigObject newCopy(ResolveStatus newStatus, + boolean ignoresFallbacks) { + return new SimpleConfigObject(origin(), value, newStatus, + ignoresFallbacks); } @Override @@ -53,6 +56,11 @@ final class SimpleConfigObject extends AbstractConfigObject { return ResolveStatus.fromBoolean(resolved); } + @Override + protected boolean ignoresFallbacks() { + return ignoresFallbacks; + } + @Override public Map unwrapped() { Map m = new HashMap(); diff --git a/src/test/scala/com/typesafe/config/impl/ConfigValueTest.scala b/src/test/scala/com/typesafe/config/impl/ConfigValueTest.scala index c01e3179..61228eaa 100644 --- a/src/test/scala/com/typesafe/config/impl/ConfigValueTest.scala +++ b/src/test/scala/com/typesafe/config/impl/ConfigValueTest.scala @@ -327,7 +327,8 @@ class ConfigValueTest extends TestUtils { // ConfigDelayedMergeObject val emptyObj = SimpleConfigObject.empty() - val dmo = new ConfigDelayedMergeObject(fakeOrigin(), List[AbstractConfigValue](emptyObj, subst("a"), subst("b")).asJava) + val dmo = new ConfigDelayedMergeObject(fakeOrigin(), + List[AbstractConfigValue](emptyObj, subst("a"), subst("b")).asJava) assertEquals(ConfigValueType.OBJECT, dmo.valueType()) unresolved { dmo.unwrapped() } unresolved { dmo.containsKey(null) }