diff --git a/HOCON.md b/HOCON.md index 9f451646..2b9561e4 100644 --- a/HOCON.md +++ b/HOCON.md @@ -423,10 +423,13 @@ i.e. it is "absolute" rather than "relative." Substitution processing is performed as the last parsing step, so a substitution can look forward in the configuration. If a configuration consists of multiple files, it may even end up -retrieving a value from another file. If a key has been specified -more than once, the substitution will always evaluate to its -latest-assigned value (the merged object or the last non-object -value that was set). +retrieving a value from another file. + +If a key has been specified more than once, the substitution will +always evaluate to its latest-assigned value (that is, it will +evaluate to the merged object, or the last non-object value that +was set, in the entire document being parsed including all +included files). If a configuration sets a value to `null` then it should not be looked up in the external source. Unfortunately there is no way to @@ -462,10 +465,98 @@ string, array, true, false, null). If the substitution is the only part of a value, then the type is preserved. Otherwise, it is value-concatenated to form a string. -Circular substitutions are invalid and should generate an error. +#### Self-Referential Substitutions -Implementations must take care, however, to allow objects to refer -to paths within themselves. For example, this must work: +The big picture: + + - substitutions normally "look forward" and use the final value + for their path expression + - when this would create a cycle, when possible the cycle must be + broken by looking backward only (thus removing one of the + substitutions that's a link in the cycle) + +The idea is to allow a new value for a field to be based on the +older value: + + path : "a:b:c" + path : ${path}":d" + +A _self-referential field_ is one which: + + - has a substitution, or value concatenation containing a + substitution, as its value + - where the substitution refers to the field being defined, + either directly or by referring to one or more other + substitutions which eventually point back to the field being + defined + +In isolation (with no merges involved), a self-referential field +is an error because the substitution cannot be resolved: + + foo : ${foo} // an error + +When `foo : ${foo}` is merged with an earlier value for `foo`, +however, the substitution can be resolved to that earlier value. +When merging two objects, the self-reference in the overriding +field refers to the overridden field. Say you have: + + foo : { a : 1 } + +and then: + + foo : ${foo} + +Then `${foo}` resolves to `{ a : 1 }`, the value of the overridden +field. + +It would be an error if these two fields were reversed, so first: + + foo : ${foo} + +and then second: + + foo : { a : 1 } + +Here the `${foo}` self-reference comes before `foo` has a value, +so it is undefined. + +Because `foo : ${foo}` conceptually looks to previous definitions +of `foo` for a value, the error should be treated as "undefined" +rather than "intractable cycle"; as a result, the optional +substitution syntax `${?foo}` does not create a cycle: + + foo : ${?foo} // this field just disappears silently + +If a substitution is hidden by a value that could not be merged +with it (by a non-object value) then it is never evaluated and no +error will be reported. So for example: + + foo : ${does-not-exist} + foo : 42 + +In this case, no matter what `${does-not-exist}` resolves to, we +know `foo` is `42`, so `${does-not-exist}` is never evaluated and +there is no error. The same is true for cycles like `foo : ${foo}, +foo : 42`, where the initial self-reference must simply be ignored. + +A self-reference resolves to the value "below" even if it's part +of a path expression. So for example: + + foo : { a : { c : 1 } } + foo : ${foo.a} + foo : { a : 2 } + +Here, `${foo.a}` would refer to `{ c : 1 }` rather than `2` and so +the final merge would be `{ a : 2, c : 1 }`. + +Recall that for a field to be self-referential, it must have a +substitution or value concatenation as its value. If a field has +an object or array value, for example, then it is not +self-referential even if there is a reference to the field itself +inside that object or array. + +Implementations must be careful to allow objects to refer to paths +within themselves, for example: bar : { foo : 42, baz : ${bar.foo} @@ -476,15 +567,43 @@ part of resolving the substitution `${bar.foo}`, there would be a cycle. The implementation must only resolve the `foo` field in `bar`, rather than recursing the entire `bar` object. -Mutually-referring objects should also work: +Because there is no inherent cycle here, the substitution must +"look forward" (including looking at the field currently being +defined). To make this clearer, `bar.baz` would be `43` in: + bar : { foo : 42, + baz : ${bar.foo} + } + bar : { foo : 43 } + +Mutually-referring objects should also work, and are not +self-referential (so they look forward): + + // bar.a should end up as 4 bar : { a : ${foo.d}, b : 1 } + bar.b = 3 + // foo.c should end up as 3 foo : { c : ${bar.b}, d : 2 } + foo.d = 4 -In general, in resolving a substitution the implementation must -lazy-evaluate the substitution target so there's no "circularity -by side effect"; only truly unresolveable circular references -should be an error. For example, this is not possible to resolve: +Another tricky case is an optional self-reference in a value +concatenation, in this example `a` should be `foo` not `foofoo` +because the self reference has to "look back" to an undefined `a`: + + a = ${?a}foo + +In general, in resolving a substitution the implementation must: + + - lazy-evaluate the substitution target so there's no + "circularity by side effect" + - "look forward" and use the final value for the path + specified in the substitution + - if a cycle results, the implementation must "look back" + in the merge stack to try to resolve the cycle + - if neither lazy evaluation nor "looking only backward" resolves + a cycle, it is an error + +For example, this is not possible to resolve: bar : ${foo} foo : ${bar} @@ -495,6 +614,23 @@ A multi-step loop like this should also be detected as invalid: b : ${c} c : ${a} +Some cases have undefined behavior because the behavior depends on +the order in which two fields are resolved, and that order is not +defined. For example: + + a : 1 + b : 2 + a : ${b} + b : ${a} + +Implementations are allowed to handle this by setting both `a` and +`b` to 1, setting both to `2`, or generating an error. Ideally +this situation would generate an error, but that may be difficult +to implement. Making the behavior defined would require always +working with ordered maps rather than unordered maps, which is too +constraining. Implementations only have to track order for +duplicate instances of the same field (i.e. merges). + ### Includes #### Include syntax diff --git a/README.md b/README.md index e54df3be..b868eb1b 100644 --- a/README.md +++ b/README.md @@ -338,20 +338,12 @@ Here are some features that might be nice to add. If you include a file and it turns out to be a directory then it would be processed in this way. - some way to merge array types. One approach could be: - `searchPath=${searchPath} ["/usr/local/foo"]`, which involves - two features: 1) substitutions referring to the key being - assigned would have to look at that key's value later in the - merge stack (rather than complaining about circularity); 2) + `searchPath=${searchPath} ["/usr/local/foo"]`, here arrays would have to be merged if a series of them appear after - a key, similar to how strings are concatenated already. A - simpler but much more limited approach would add `+=` as an - alternative to `:`/`=`, where `+=` would append an array value - to the array's previous value. (Note that regular `=` already - merges object values, to avoid object merge you have to first - set the object to a non-object such as null, then set a new - object. For consistency, if there's "array concatenation" - within one value, maybe objects should also be able to merge - within one value.) + a key, similar to how strings are concatenated already. + For consistency, maybe objects would also support this + syntax, though there's an existing way to merge objects + (duplicate fields). - including URLs (which would allow forcing file: when inside a classpath resource, among other things) 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 16b68ad0..51a1e410 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -12,6 +12,7 @@ import java.util.List; import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigOrigin; import com.typesafe.config.ConfigValueType; +import com.typesafe.config.impl.ResolveReplacer.Undefined; /** * The issue here is that we want to first merge our stack of config files, and @@ -21,8 +22,8 @@ import com.typesafe.config.ConfigValueType; * stack of values that should be merged, and resolve the merge when we evaluate * substitutions. */ -final class ConfigDelayedMerge extends AbstractConfigValue implements - Unmergeable { +final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeable, + ReplaceableMergeStack { private static final long serialVersionUID = 1L; @@ -65,32 +66,88 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements @Override AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, ResolveContext context) throws NotPossibleToResolve, NeedsFullResolve { - return resolveSubstitutions(stack, resolver, context); + return resolveSubstitutions(this, stack, resolver, context); } // static method also used by ConfigDelayedMergeObject - static AbstractConfigValue resolveSubstitutions(List stack, - SubstitutionResolver resolver, ResolveContext context) throws NotPossibleToResolve, - NeedsFullResolve { + static AbstractConfigValue resolveSubstitutions(ReplaceableMergeStack replaceable, + List stack, SubstitutionResolver resolver, ResolveContext context) + throws NotPossibleToResolve, NeedsFullResolve { // to resolve substitutions, we need to recursively resolve // the stack of stuff to merge, and merge the stack so // we won't be a delayed merge anymore. If restrictToChildOrNull // is non-null, we may remain a delayed merge though. + int count = 0; AbstractConfigValue merged = null; for (AbstractConfigValue v : stack) { - AbstractConfigValue resolved = resolver.resolve(v, context); + boolean replaced = false; + // checking for RESOLVED already is just an optimization + // to avoid creating the replacer when it can't possibly + // be needed. + if (v.resolveStatus() != ResolveStatus.RESOLVED) { + // If, while resolving 'v' we come back to the same + // merge stack, we only want to look _below_ 'v' + // in the stack. So we arrange to replace the + // ConfigDelayedMerge with a value that is only + // the remainder of the stack below this one. + + context.replace((AbstractConfigValue) replaceable, + replaceable.makeReplacer(count + 1)); + replaced = true; + } + + AbstractConfigValue resolved; + try { + resolved = resolver.resolve(v, context); + } finally { + if (replaced) + context.unreplace((AbstractConfigValue) replaceable); + } + if (resolved != null) { if (merged == null) merged = resolved; else merged = merged.withFallback(resolved); } + count += 1; } return merged; } + @Override + public ResolveReplacer makeReplacer(final int skipping) { + return new ResolveReplacer() { + @Override + protected AbstractConfigValue makeReplacement() throws Undefined { + return ConfigDelayedMerge.makeReplacement(stack, skipping); + } + }; + } + + // static method also used by ConfigDelayedMergeObject + static AbstractConfigValue makeReplacement(List stack, int skipping) + throws Undefined { + + List subStack = stack.subList(skipping, stack.size()); + + if (subStack.isEmpty()) { + throw new ResolveReplacer.Undefined(); + } else { + // generate a new merge stack from only the remaining items + AbstractConfigValue merged = null; + for (AbstractConfigValue v : subStack) { + if (merged == null) + merged = v; + else + merged = merged.withFallback(v); + } + return merged; + } + } + @Override ResolveStatus resolveStatus() { return ResolveStatus.UNRESOLVED; @@ -131,6 +188,11 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements @Override protected final ConfigDelayedMerge mergedWithObject(AbstractConfigObject fallback) { + return mergedWithNonObject(fallback); + } + + @Override + protected ConfigDelayedMerge mergedWithNonObject(AbstractConfigValue fallback) { if (ignoresFallbacks) throw new ConfigException.BugOrBroken("should not be reached"); diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java index 30445822..60c3238b 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -17,8 +17,8 @@ import com.typesafe.config.ConfigValue; // This is just like ConfigDelayedMerge except we know statically // that it will turn out to be an object. -final class ConfigDelayedMergeObject extends AbstractConfigObject implements - Unmergeable { +final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unmergeable, + ReplaceableMergeStack { private static final long serialVersionUID = 1L; @@ -62,7 +62,7 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements @Override AbstractConfigObject resolveSubstitutions(SubstitutionResolver resolver, ResolveContext context) throws NotPossibleToResolve, NeedsFullResolve { - AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(stack, resolver, + AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(this, stack, resolver, context); if (merged instanceof AbstractConfigObject) { return (AbstractConfigObject) merged; @@ -72,6 +72,16 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements } } + @Override + public ResolveReplacer makeReplacer(final int skipping) { + return new ResolveReplacer() { + @Override + protected AbstractConfigValue makeReplacement() throws Undefined { + return ConfigDelayedMerge.makeReplacement(stack, skipping); + } + }; + } + @Override ResolveStatus resolveStatus() { return ResolveStatus.UNRESOLVED; @@ -94,11 +104,14 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements @Override protected ConfigDelayedMergeObject mergedWithObject(AbstractConfigObject fallback) { + return mergedWithNonObject(fallback); + } + + @Override + protected ConfigDelayedMergeObject mergedWithNonObject(AbstractConfigValue fallback) { if (ignoresFallbacks) throw new ConfigException.BugOrBroken("should not be reached"); - // since we are an object, and the fallback is, we'll need to - // merge the fallback once we resolve. List newStack = new ArrayList(); newStack.addAll(stack); newStack.add(fallback); 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 6d9140cf..daeb76f2 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -132,120 +132,143 @@ final class ConfigSubstitution extends AbstractConfigValue implements return pieces; } - private static AbstractConfigValue findInObject(AbstractConfigObject root, - SubstitutionResolver resolver, /* null if we should not have refs */ - Path subst, ResolveContext context) - throws NotPossibleToResolve, NeedsFullResolve { + /** resolver is null if we should not have refs */ + private AbstractConfigValue findInObject(final AbstractConfigObject root, + final SubstitutionResolver resolver, final SubstitutionExpression subst, + final ResolveContext context) throws NotPossibleToResolve, NeedsFullResolve { + return context.traversing(this, subst, new ResolveContext.Resolver() { + @Override + public AbstractConfigValue call() throws NotPossibleToResolve, NeedsFullResolve { + return root.peekPath(subst.path(), resolver, context); + } + }); + } - AbstractConfigValue result = root.peekPath(subst, resolver, context); + private AbstractConfigValue resolve(final SubstitutionResolver resolver, + final SubstitutionExpression subst, final ResolveContext context) + throws NotPossibleToResolve, + NeedsFullResolve { + // First we look up the full path, which means relative to the + // included file if we were not a root file + AbstractConfigValue result = findInObject(resolver.root(), resolver, subst, context); + + if (result == null) { + // Then we want to check relative to the root file. We don't + // want the prefix we were included at to be used when looking + // up env variables either. + SubstitutionExpression unprefixed = subst + .changePath(subst.path().subPath(prefixLength)); + + if (result == null && prefixLength > 0) { + result = findInObject(resolver.root(), resolver, unprefixed, context); + } + + if (result == null && context.options().getUseSystemEnvironment()) { + result = findInObject(ConfigImpl.envVariablesAsConfigObject(), null, unprefixed, + context); + } + } + + if (result != null) { + final AbstractConfigValue unresolved = result; + result = context.traversing(this, subst, new ResolveContext.Resolver() { + @Override + public AbstractConfigValue call() throws NotPossibleToResolve, NeedsFullResolve { + return resolver.resolve(unresolved, context); + } + }); + } return result; } - private AbstractConfigValue resolve(SubstitutionResolver resolver, - SubstitutionExpression subst, ResolveContext context) throws NotPossibleToResolve, - NeedsFullResolve { - context.traverse(this, subst.path()); - - try { - - // First we look up the full path, which means relative to the - // included file if we were not a root file - AbstractConfigValue result = findInObject(resolver.root(), resolver, subst.path(), - context); - - if (result == null) { - // Then we want to check relative to the root file. We don't - // want the prefix we were included at to be used when looking - // up - // env variables either. - Path unprefixed = subst.path().subPath(prefixLength); - - if (result == null && prefixLength > 0) { - result = findInObject(resolver.root(), resolver, unprefixed, context); - } - - if (result == null && context.options().getUseSystemEnvironment()) { - result = findInObject(ConfigImpl.envVariablesAsConfigObject(), null, - unprefixed, context); - } - } - - if (result != null) { - result = resolver.resolve(result, context); - } - - return result; - - } finally { - context.untraverse(this); + private static ResolveReplacer undefinedReplacer = new ResolveReplacer() { + @Override + protected AbstractConfigValue makeReplacement() throws Undefined { + throw new Undefined(); } + }; + + private AbstractConfigValue resolveValueConcat(SubstitutionResolver resolver, + ResolveContext context) throws NotPossibleToResolve { + // need to concat everything into a string + StringBuilder sb = new StringBuilder(); + for (Object p : pieces) { + if (p instanceof String) { + sb.append((String) p); + } else { + SubstitutionExpression exp = (SubstitutionExpression) p; + ConfigValue v; + try { + // to concat into a string we have to do a full resolve, + // so unrestrict the context + v = resolve(resolver, exp, context.unrestricted()); + } catch (NeedsFullResolve e) { + throw new NotPossibleToResolve(null, exp.path().render(), + "Some kind of loop or interdependency prevents resolving " + exp, e); + } + + if (v == null) { + if (exp.optional()) { + // append nothing to StringBuilder + } else { + throw new ConfigException.UnresolvedSubstitution(origin(), exp.toString()); + } + } else { + switch (v.valueType()) { + case LIST: + case OBJECT: + // cannot substitute lists and objects into strings + throw new ConfigException.WrongType(v.origin(), exp.path().render(), + "not a list or object", v.valueType().name()); + default: + sb.append(((AbstractConfigValue) v).transformToString()); + } + } + } + } + return new ConfigString(origin(), sb.toString()); } - private ConfigValue resolve(SubstitutionResolver resolver, ResolveContext context) + private AbstractConfigValue resolveSingleSubst(SubstitutionResolver resolver, + ResolveContext context) throws NotPossibleToResolve { - if (pieces.size() > 1) { - // need to concat everything into a string - StringBuilder sb = new StringBuilder(); - for (Object p : pieces) { - if (p instanceof String) { - sb.append((String) p); - } else { - SubstitutionExpression exp = (SubstitutionExpression) p; - ConfigValue v; - try { - // to concat into a string we have to do a full resolve, - // so unrestrict the context - v = resolve(resolver, exp, context.unrestricted()); - } catch (NeedsFullResolve e) { - throw new NotPossibleToResolve(null, exp.path().render(), - "Some kind of loop or interdependency prevents resolving " + exp, e); - } - if (v == null) { - if (exp.optional()) { - // append nothing to StringBuilder - } else { - throw new ConfigException.UnresolvedSubstitution(origin(), - exp.toString()); - } - } else { - switch (v.valueType()) { - case LIST: - case OBJECT: - // cannot substitute lists and objects into strings - throw new ConfigException.WrongType(v.origin(), exp.path().render(), - "not a list or object", v.valueType().name()); - default: - sb.append(((AbstractConfigValue) v).transformToString()); - } - } - } - } - return new ConfigString(origin(), sb.toString()); - } else { - if (!(pieces.get(0) instanceof SubstitutionExpression)) - throw new ConfigException.BugOrBroken( - "ConfigSubstitution should never contain a single String piece"); - SubstitutionExpression exp = (SubstitutionExpression) pieces.get(0); - ConfigValue v; - try { - v = resolve(resolver, exp, context); - } catch (NeedsFullResolve e) { - throw new NotPossibleToResolve(null, exp.path().render(), - "Some kind of loop or interdependency prevents resolving " + exp, e); - } - if (v == null && !exp.optional()) { - throw new ConfigException.UnresolvedSubstitution(origin(), exp.toString()); - } - return v; + if (!(pieces.get(0) instanceof SubstitutionExpression)) + throw new ConfigException.BugOrBroken( + "ConfigSubstitution should never contain a single String piece"); + + SubstitutionExpression exp = (SubstitutionExpression) pieces.get(0); + AbstractConfigValue v; + try { + v = resolve(resolver, exp, context); + } catch (NeedsFullResolve e) { + throw new NotPossibleToResolve(null, exp.path().render(), + "Some kind of loop or interdependency prevents resolving " + exp, e); } + if (v == null && !exp.optional()) { + throw new ConfigException.UnresolvedSubstitution(origin(), exp.toString()); + } + return v; } @Override AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, ResolveContext context) throws NotPossibleToResolve { - AbstractConfigValue resolved = (AbstractConfigValue) resolve(resolver, context); + AbstractConfigValue resolved; + if (pieces.size() > 1) { + // if you have "foo = ${?foo}bar" then we will + // self-referentially look up foo and we need to + // get undefined, rather than "bar" + context.replace(this, undefinedReplacer); + try { + resolved = resolveValueConcat(resolver, context); + } finally { + context.unreplace(this); + } + } else { + resolved = resolveSingleSubst(resolver, context); + } return resolved; } diff --git a/config/src/main/java/com/typesafe/config/impl/MemoKey.java b/config/src/main/java/com/typesafe/config/impl/MemoKey.java index 7237234f..81f0ae7c 100644 --- a/config/src/main/java/com/typesafe/config/impl/MemoKey.java +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -10,6 +10,10 @@ final class MemoKey { final private AbstractConfigValue value; final private Path restrictToChildOrNull; + AbstractConfigValue value() { + return value; + } + @Override public final int hashCode() { int h = System.identityHashCode(value); diff --git a/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java b/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java new file mode 100644 index 00000000..7e5dc9d1 --- /dev/null +++ b/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java @@ -0,0 +1,14 @@ +package com.typesafe.config.impl; + +/** + * Implemented by a merge stack (ConfigDelayedMerge, ConfigDelayedMergeObject) + * that replaces itself during substitution resolution in order to implement + * "look backwards only" semantics. + */ +interface ReplaceableMergeStack { + /** + * Make a replacer for this object, skipping the given number of items in + * the stack. + */ + ResolveReplacer makeReplacer(int skipping); +} diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java index 0c1635a7..1782faa6 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -1,47 +1,131 @@ package com.typesafe.config.impl; +import java.util.Collections; +import java.util.HashMap; +import java.util.LinkedList; +import java.util.Map; import java.util.Set; import java.util.LinkedHashSet; +import java.util.concurrent.Callable; import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigResolveOptions; +import com.typesafe.config.impl.AbstractConfigValue.NeedsFullResolve; +import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; import com.typesafe.config.impl.AbstractConfigValue.SelfReferential; +import com.typesafe.config.impl.ResolveReplacer.Undefined; final class ResolveContext { + // Resolves that we have already begun (for cycle detection). + // SubstitutionResolve separately memoizes completed resolves. // this set is unfortunately mutable and the user of ResolveContext // has to be sure it's only shared between ResolveContext that // are in the same traversal. - final private Set traversed; + final private LinkedList> traversedStack; final private ConfigResolveOptions options; - final private Path restrictToChild; // can be null + // the current path restriction, used to ensure lazy + // resolution and avoid gratuitous cycles. + // CAN BE NULL for a full resolve. + final private Path restrictToChild; + // if we try to resolve something in here, use the + // given replacement instead. + final private Map> replacements; - ResolveContext(Set traversed, ConfigResolveOptions options, Path restrictToChild) { - this.traversed = traversed; + ResolveContext(LinkedList> traversedStack, ConfigResolveOptions options, + Path restrictToChild, + Map> replacements) { + this.traversedStack = traversedStack; this.options = options; this.restrictToChild = restrictToChild; + this.replacements = replacements; } ResolveContext(ConfigResolveOptions options, Path restrictToChild) { // LinkedHashSet keeps the traversal order which is at least useful // in error messages if nothing else - this(new LinkedHashSet(), options, restrictToChild); + this(new LinkedList>(Collections.singletonList(new LinkedHashSet())), + options, restrictToChild, new HashMap>()); } - void traverse(ConfigSubstitution value, Path via) throws SelfReferential { + private void traverse(ConfigSubstitution value, SubstitutionExpression via) + throws SelfReferential { + Set traversed = traversedStack.peekFirst(); + MemoKey key = new MemoKey(value, restrictToChild); - if (traversed.contains(key)) - throw new SelfReferential(value.origin(), via.render()); + if (traversed.contains(key)) { + throw new SelfReferential(value.origin(), via.path().render()); + } traversed.add(key); } - void untraverse(ConfigSubstitution value) { + private void untraverse(ConfigSubstitution value) { + Set traversed = traversedStack.peekFirst(); + MemoKey key = new MemoKey(value, restrictToChild); if (!traversed.remove(key)) throw new ConfigException.BugOrBroken( "untraverse() did not find the untraversed substitution " + value); } + // this just exists to fix the "throws Exception" on Callable + interface Resolver extends Callable { + @Override + AbstractConfigValue call() throws NotPossibleToResolve, NeedsFullResolve; + } + + AbstractConfigValue traversing(ConfigSubstitution value, SubstitutionExpression subst, + Resolver callable) throws NotPossibleToResolve, NeedsFullResolve { + try { + traverse(value, subst); + } catch (SelfReferential e) { + if (subst.optional()) + return null; + else + throw e; + } + + try { + return callable.call(); + } finally { + untraverse(value); + } + } + + void replace(AbstractConfigValue value, ResolveReplacer replacer) { + MemoKey key = new MemoKey(value, null /* restrictToChild */); + LinkedList stack = replacements.get(key); + if (stack == null) { + stack = new LinkedList(); + replacements.put(key, stack); + } + stack.addFirst(replacer); + // we have to reset the cycle detection because with the + // replacement, a cycle may be broken + traversedStack.addFirst(new LinkedHashSet()); + } + + void unreplace(AbstractConfigValue value) { + MemoKey key = new MemoKey(value, null /* restrictToChild */); + LinkedList stack = replacements.get(key); + if (stack == null) + throw new ConfigException.BugOrBroken("unreplace() without replace(): " + value); + + stack.removeFirst(); + Set oldTraversed = traversedStack.removeFirst(); + if (!oldTraversed.isEmpty()) + throw new ConfigException.BugOrBroken( + "unreplace() with stuff still in the traverse set: " + oldTraversed); + } + + AbstractConfigValue replacement(MemoKey key) throws Undefined { + LinkedList stack = replacements.get(new MemoKey(key.value(), null)); + if (stack == null || stack.isEmpty()) + return key.value(); + else + return stack.peek().replace(); + } + ConfigResolveOptions options() { return options; } @@ -58,7 +142,7 @@ final class ResolveContext { if (restrictTo == restrictToChild) return this; else - return new ResolveContext(traversed, options, restrictTo); + return new ResolveContext(traversedStack, options, restrictTo, replacements); } ResolveContext unrestricted() { diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java new file mode 100644 index 00000000..846d058e --- /dev/null +++ b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java @@ -0,0 +1,27 @@ +package com.typesafe.config.impl; + +/** Callback that generates a replacement to use for resolving a substitution. */ +abstract class ResolveReplacer { + static final class Undefined extends Exception { + private static final long serialVersionUID = 1L; + + Undefined() { + super("No replacement, substitution will resolve to undefined"); + } + } + + // this is a "lazy val" in essence (we only want to + // make the replacement one time). Making it volatile + // is good enough for thread safety as long as this + // cache is only an optimization and making the replacement + // twice has no side effects, which it should not... + private volatile AbstractConfigValue replacement = null; + + final AbstractConfigValue replace() throws Undefined { + if (replacement == null) + replacement = makeReplacement(); + return replacement; + } + + protected abstract AbstractConfigValue makeReplacement() throws Undefined; +} diff --git a/config/src/main/java/com/typesafe/config/impl/SubstitutionExpression.java b/config/src/main/java/com/typesafe/config/impl/SubstitutionExpression.java index be67073a..c5efe654 100644 --- a/config/src/main/java/com/typesafe/config/impl/SubstitutionExpression.java +++ b/config/src/main/java/com/typesafe/config/impl/SubstitutionExpression.java @@ -23,7 +23,10 @@ final class SubstitutionExpression implements Serializable { } SubstitutionExpression changePath(Path newPath) { - return new SubstitutionExpression(newPath, optional); + if (newPath == path) + return this; + else + return new SubstitutionExpression(newPath, optional); } @Override 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 a4909d0e..380a611c 100644 --- a/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java +++ b/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java @@ -10,6 +10,7 @@ import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigResolveOptions; import com.typesafe.config.impl.AbstractConfigValue.NeedsFullResolve; import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; +import com.typesafe.config.impl.ResolveReplacer.Undefined; /** * This exists because we have to memoize resolved substitutions as we go @@ -48,31 +49,52 @@ final class SubstitutionResolver { if (cached != null) { return cached; } else { - AbstractConfigValue resolved = original.resolveSubstitutions(this, context); + MemoKey key = restrictedKey != null ? restrictedKey : fullKey; - if (resolved == null || resolved.resolveStatus() == ResolveStatus.RESOLVED) { - // if the resolved object is fully resolved by resolving - // only the restrictToChildOrNull, then it can be cached - // under fullKey since the child we were restricted to - // turned out to be the only unresolved thing. - memos.put(fullKey, resolved); - } else { - // if we have an unresolved object then either we did a - // partial resolve restricted to a certain child, or it's - // a bug. - if (context.isRestrictedToChild()) { - if (restrictedKey == null) { - throw new ConfigException.BugOrBroken( - "restrictedKey should not be null here"); - } - memos.put(restrictedKey, resolved); - } else { - throw new ConfigException.BugOrBroken( - "resolveSubstitutions() did not give us a resolved object"); - } + AbstractConfigValue replacement; + boolean forceUndefined = false; + try { + replacement = context.replacement(key); + } catch (Undefined e) { + replacement = original; + forceUndefined = true; } - return resolved; + if (replacement != original) { + // start over, checking if replacement was memoized + return resolve(replacement, context); + } else { + AbstractConfigValue resolved; + + if (forceUndefined) + resolved = null; + else + resolved = original.resolveSubstitutions(this, context); + + if (resolved == null || resolved.resolveStatus() == ResolveStatus.RESOLVED) { + // if the resolved object is fully resolved by resolving + // only the restrictToChildOrNull, then it can be cached + // under fullKey since the child we were restricted to + // turned out to be the only unresolved thing. + memos.put(fullKey, resolved); + } else { + // if we have an unresolved object then either we did a + // partial resolve restricted to a certain child, or it's + // a bug. + if (context.isRestrictedToChild()) { + if (restrictedKey == null) { + throw new ConfigException.BugOrBroken( + "restrictedKey should not be null here"); + } + memos.put(restrictedKey, resolved); + } else { + throw new ConfigException.BugOrBroken( + "resolveSubstitutions() did not give us a resolved object"); + } + } + + return resolved; + } } } @@ -84,15 +106,18 @@ final class SubstitutionResolver { ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { SubstitutionResolver resolver = new SubstitutionResolver(root); + ResolveContext context = new ResolveContext(options, restrictToChildOrNull); - return resolver.resolve(value, new ResolveContext(options, restrictToChildOrNull)); + return resolver.resolve(value, context); } static AbstractConfigValue resolveWithExternalExceptions(AbstractConfigValue value, AbstractConfigObject root, ConfigResolveOptions options) { SubstitutionResolver resolver = new SubstitutionResolver(root); + ResolveContext context = new ResolveContext(options, null /* restrictToChild */); + try { - return resolver.resolve(value, new ResolveContext(options, null /* restrictToChild */)); + return resolver.resolve(value, context); } catch (NotPossibleToResolve e) { throw e.exportException(value.origin(), null); } catch (NeedsFullResolve e) { 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 440d9ca5..984dc0d0 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -198,6 +198,13 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(intValue(57), v) } + @Test + def substitutionsLookForward() { + val obj = parseObject("""a=1,b=${a},a=2""") + val resolved = resolve(obj) + assertEquals(2, resolved.getInt("b")) + } + private val substCycleObject = { parseObject(""" { @@ -214,7 +221,54 @@ class ConfigSubstitutionTest extends TestUtils { val e = intercept[ConfigException.BadValue] { val v = resolveWithoutFallbacks(s, substCycleObject) } - assertTrue(e.getMessage().contains("cycle")) + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle")) + } + + @Test + def throwOnOptionalReferenceToNonOptionalCycle() { + // we look up ${?foo}, but the cycle has hard + // non-optional links in it so still has to throw. + val s = subst("foo", optional = true) + val e = intercept[ConfigException.BadValue] { + val v = resolveWithoutFallbacks(s, substCycleObject) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle")) + } + + // ALL the links have to be optional here for the cycle to be ignored + private val substCycleObjectOptionalLink = { + parseObject(""" +{ + "foo" : ${?bar}, + "bar" : ${?a.b.c}, + "a" : { "b" : { "c" : ${?foo} } } +} +""") + } + + @Test + def optionalLinkCyclesActLikeUndefined() { + val s = subst("foo", optional = true) + val v = resolveWithoutFallbacks(s, substCycleObjectOptionalLink) + assertNull("Cycle with optional links in it resolves to null if it's a cycle", v) + } + + @Test + def throwOnTwoKeyCycle() { + val obj = parseObject("""a:${b},b:${a}""") + val e = intercept[ConfigException.BadValue] { + resolve(obj) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle")) + } + + @Test + def throwOnFourKeyCycle() { + val obj = parseObject("""a:${b},b:${c},c:${d},d:${a}""") + val e = intercept[ConfigException.BadValue] { + resolve(obj) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle")) } @Test @@ -249,6 +303,33 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(42, resolved.getInt("a.cycle")) } + @Test + def ignoreHiddenUndefinedSubst() { + // if a substitution is overridden then it shouldn't matter that it's undefined + val obj = parseObject("""a=${nonexistent},a=42""") + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("a")) + } + + @Test + def objectDoesNotHideUndefinedSubst() { + // if a substitution is overridden by an object we still need to + // evaluate the substitution + val obj = parseObject("""a=${nonexistent},a={ b : 42 }""") + val e = intercept[ConfigException.UnresolvedSubstitution] { + resolve(obj) + } + assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("Could not resolve")) + } + + @Test + def ignoreHiddenCircularSubst() { + // if a substitution is overridden then it shouldn't matter that it's circular + val obj = parseObject("""a=${a},a=42""") + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("a")) + } + private val delayedMergeObjectResolveProblem1 = { parseObject(""" defaults { @@ -305,22 +386,24 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(43, resolved.getInt("item2.b.c")) } - // this case has to traverse ${defaults} twice, once - // trying to resolve it all and then as part of that - // trying a partial resolve of only b.c - // thus a simple cycle-detector would get confused - // and think that defaults was a cycle. + // in this case, item1 is self-referential because + // it refers to ${defaults} which refers back to + // ${item1}. When self-referencing, only the + // value of ${item1} "looking back" should be + // visible. This is really a test of the + // self-referencing semantics. private val delayedMergeObjectResolveProblem3 = { parseObject(""" + item1.b.c = 100 defaults { - // we depend on item1.b.c, creating a cycle that can be handled + // we depend on item1.b.c a = ${item1.b.c} b = 2 } // make item1 into a ConfigDelayedMergeObject item1 = ${defaults} - // note that we'll resolve to an object value - // so item1.b will depend on also looking up ${defaults} + // the ${item1.b.c} above in ${defaults} should ignore + // this because it only looks back item1.b = { c : 43 } // be sure we can resolve a substitution to a value in // a delayed-merge object. @@ -337,7 +420,7 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(parseObject("{ c : 43 }"), resolved.getObject("item1.b")) assertEquals(43, resolved.getInt("item1.b.c")) assertEquals(43, resolved.getInt("item2.b.c")) - assertEquals(43, resolved.getInt("defaults.a")) + assertEquals(100, resolved.getInt("defaults.a")) } private val delayedMergeObjectResolveProblem4 = { @@ -851,4 +934,233 @@ class ConfigSubstitutionTest extends TestUtils { val resolved = resolve(obj) assertEquals(Seq(1, 2, 3, 4), resolved.getIntList("a").asScala) } + + @Test + def substSelfReference() { + val obj = parseObject("""a=1, a=${a}""") + val resolved = resolve(obj) + assertEquals(1, resolved.getInt("a")) + } + + @Test + def substSelfReferenceUndefined() { + val obj = parseObject("""a=${a}""") + val e = intercept[ConfigException.BadValue] { + resolve(obj) + } + assertTrue("wrong exception: " + e.getMessage, e.getMessage.contains("cycle")) + } + + @Test + def substSelfReferenceOptional() { + val obj = parseObject("""a=${?a}""") + val resolved = resolve(obj) + assertEquals("optional self reference disappears", 0, resolved.root.size) + } + + @Test + def substSelfReferenceIndirect() { + val obj = parseObject("""a=1, b=${a}, a=${b}""") + val resolved = resolve(obj) + assertEquals(1, resolved.getInt("a")) + } + + @Test + def substSelfReferenceDoubleIndirect() { + val obj = parseObject("""a=1, b=${c}, c=${a}, a=${b}""") + val resolved = resolve(obj) + assertEquals(1, resolved.getInt("a")) + } + + @Test + def substSelfReferenceIndirectStackCycle() { + // this situation is undefined, depends on + // whether we resolve a or b first. + val obj = parseObject("""a=1, b={c=5}, b=${a}, a=${b}""") + val resolved = resolve(obj) + val option1 = parseObject(""" b={c=5}, a={c=5} """).toConfig() + val option2 = parseObject(""" b=1, a=1 """).toConfig() + assertTrue("not an expected possibility: " + resolved + + " expected 1: " + option1 + " or 2: " + option2, + resolved == option1 || resolved == option2) + } + + @Test + def substSelfReferenceObject() { + val obj = parseObject("""a={b=5}, a=${a}""") + val resolved = resolve(obj) + assertEquals(5, resolved.getInt("a.b")) + } + + @Test + def substSelfReferenceInConcat() { + val obj = parseObject("""a=1, a=${a}foo""") + val resolved = resolve(obj) + assertEquals("1foo", resolved.getString("a")) + } + + @Test + def substSelfReferenceIndirectInConcat() { + // this situation is undefined, depends on + // whether we resolve a or b first. If b first + // then there's an error because ${a} is undefined. + // if a first then b=1foo and a=1foo. + val obj = parseObject("""a=1, b=${a}foo, a=${b}""") + val either = try { + Left(resolve(obj)) + } catch { + case e: ConfigException.UnresolvedSubstitution => + Right(e) + } + val option1 = Left(parseObject("""a:1foo,b:1foo""").toConfig) + assertTrue("not an expected possibility: " + either + + " expected value " + option1 + " or an exception", + either == option1 || either.isRight) + } + + @Test + def substOptionalSelfReferenceInConcat() { + val obj = parseObject("""a=${?a}foo""") + val resolved = resolve(obj) + assertEquals("foo", resolved.getString("a")) + } + + @Test + def substSelfReferenceMiddleOfStack() { + val obj = parseObject("""a=1, a=${a}, a=2""") + val resolved = resolve(obj) + // the substitution would be 1, but then 2 overrides + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substSelfReferenceObjectMiddleOfStack() { + val obj = parseObject("""a={b=5}, a=${a}, a={c=6}""") + val resolved = resolve(obj) + assertEquals(5, resolved.getInt("a.b")) + assertEquals(6, resolved.getInt("a.c")) + } + + @Test + def substOptionalSelfReferenceMiddleOfStack() { + val obj = parseObject("""a=1, a=${?a}, a=2""") + val resolved = resolve(obj) + // the substitution would be 1, but then 2 overrides + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substSelfReferenceBottomOfStack() { + // self-reference should just be ignored since it's + // overridden + val obj = parseObject("""a=${a}, a=1, a=2""") + val resolved = resolve(obj) + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substOptionalSelfReferenceBottomOfStack() { + val obj = parseObject("""a=${?a}, a=1, a=2""") + val resolved = resolve(obj) + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substSelfReferenceTopOfStack() { + val obj = parseObject("""a=1, a=2, a=${a}""") + val resolved = resolve(obj) + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substOptionalSelfReferenceTopOfStack() { + val obj = parseObject("""a=1, a=2, a=${?a}""") + val resolved = resolve(obj) + assertEquals(2, resolved.getInt("a")) + } + + @Test + def substSelfReferenceAlongAPath() { + // ${a} in the middle of the stack means "${a} in the stack + // below us" and so ${a.b} means b inside the "${a} below us" + // not b inside the final "${a}" + val obj = parseObject("""a={b={c=5}}, a=${a.b}, a={b=2}""") + val resolved = resolve(obj) + assertEquals(5, resolved.getInt("a.c")) + } + + @Test + def substInChildFieldNotASelfReference1() { + // here, ${bar.foo} is not a self reference because + // it's the value of a child field of bar, not bar + // itself; so we use bar's current value, rather than + // looking back in the merge stack + val obj = parseObject(""" + bar : { foo : 42, + baz : ${bar.foo} + } + """) + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("bar.baz")) + assertEquals(42, resolved.getInt("bar.foo")) + } + + @Test + def substInChildFieldNotASelfReference2() { + // checking that having bar.foo later in the stack + // doesn't break the behavior + val obj = parseObject(""" + bar : { foo : 42, + baz : ${bar.foo} + } + bar : { foo : 43 } + """) + val resolved = resolve(obj) + assertEquals(43, resolved.getInt("bar.baz")) + assertEquals(43, resolved.getInt("bar.foo")) + } + + @Test + def substInChildFieldNotASelfReference3() { + // checking that having bar.foo earlier in the merge + // stack doesn't break the behavior + val obj = parseObject(""" + bar : { foo : 43 } + bar : { foo : 42, + baz : ${bar.foo} + } + """) + val resolved = resolve(obj) + assertEquals(42, resolved.getInt("bar.baz")) + assertEquals(42, resolved.getInt("bar.foo")) + } + + @Test + def mutuallyReferringNotASelfReference() { + val obj = parseObject(""" + // bar.a should end up as 4 + bar : { a : ${foo.d}, b : 1 } + bar.b = 3 + // foo.c should end up as 3 + foo : { c : ${bar.b}, d : 2 } + foo.d = 4 + """) + val resolved = resolve(obj) + assertEquals(4, resolved.getInt("bar.a")) + assertEquals(3, resolved.getInt("foo.c")) + } + + @Test + def substSelfReferenceMultipleTimes() { + val obj = parseObject("""a=1,a=${a},a=${a},a=${a}""") + val resolved = resolve(obj) + assertEquals(1, resolved.getInt("a")) + } + + @Test + def substSelfReferenceInConcatMultipleTimes() { + val obj = parseObject("""a=1,a=${a}x,a=${a}y,a=${a}z""") + val resolved = resolve(obj) + assertEquals("1xyz", resolved.getString("a")) + } } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConfigTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConfigTest.scala index 1478729b..8973e55c 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigTest.scala @@ -369,10 +369,12 @@ class ConfigTest extends TestUtils { val fixUpCycle = parseObject(""" { "a" : { "b" : { "c" : { "q" : "u" } } } } """) val merged = mergeUnresolved(fixUpCycle, cycleObject) - val e2 = intercept[ConfigException.BadValue] { + val e2 = intercept[ConfigException.UnresolvedSubstitution] { val v = resolveNoSystem(subst("foo"), merged) } - assertTrue(e2.getMessage().contains("cycle")) + // TODO: it would be nicer if the above threw BadValue with an + // explanation about the cycle. + //assertTrue(e2.getMessage().contains("cycle")) } @Test