From 91497e4a1e3937964856535702d23394d1e33065 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 4 Jul 2014 14:34:02 -0400 Subject: [PATCH 01/14] Add config.trace=substitutions mode Dumps a bunch of verbose output about how substitutions are being resolved. --- .../config/impl/ConfigConcatenation.java | 12 +++++++ .../config/impl/ConfigDelayedMerge.java | 29 +++++++++++++++-- .../com/typesafe/config/impl/ConfigImpl.java | 25 +++++++++++++++ .../typesafe/config/impl/ConfigReference.java | 3 ++ .../com/typesafe/config/impl/MemoKey.java | 5 +++ .../typesafe/config/impl/ResolveContext.java | 32 ++++++++++++++++++- .../typesafe/config/impl/ResolveReplacer.java | 2 ++ .../typesafe/config/impl/ResolveSource.java | 28 +++++++++++++++- 8 files changed, 132 insertions(+), 4 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java index e592a250..8cb15753 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java @@ -171,11 +171,23 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab @Override AbstractConfigValue resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve { + if (ConfigImpl.traceSubstitutionsEnabled()) { + int indent = context.depth() + 2; + ConfigImpl.trace(indent - 1, "concatenation has " + pieces.size() + " pieces:"); + int count = 0; + for (AbstractConfigValue v : pieces) { + ConfigImpl.trace(indent, count + ": " + v); + count += 1; + } + } + List resolved = new ArrayList(pieces.size()); for (AbstractConfigValue p : pieces) { // to concat into a string we have to do a full resolve, // so unrestrict the context AbstractConfigValue r = context.unrestricted().resolve(p); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "resolved concat piece to " + r); if (r == null) { // it was optional... omit } else { 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 28153331..df942d5b 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -62,6 +62,16 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // static method also used by ConfigDelayedMergeObject static AbstractConfigValue resolveSubstitutions(ReplaceableMergeStack replaceable, List stack, ResolveContext context) throws NotPossibleToResolve { + if (ConfigImpl.traceSubstitutionsEnabled()) { + int indent = context.depth() + 2; + ConfigImpl.trace(indent - 1, "delayed merge stack has " + stack.size() + " items:"); + int count = 0; + for (AbstractConfigValue v : stack) { + ConfigImpl.trace(indent, count + ": " + v); + count += 1; + } + } + // 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 @@ -86,6 +96,10 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // ConfigDelayedMerge with a value that is only // the remainder of the stack below this one. + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth() + 1, "because item " + count + + " in this stack is unresolved, resolving it can only look at remaining " + + (stack.size() - count - 1) + " items"); context.source().replace((AbstractConfigValue) replaceable, replaceable.makeReplacer(count + 1)); replaced = true; @@ -93,6 +107,9 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl AbstractConfigValue resolved; try { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth() + 1, "resolving item " + count + " in merge stack of " + + stack.size()); resolved = context.resolve(v); } finally { if (replaced) @@ -100,14 +117,20 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl } if (resolved != null) { - if (merged == null) + if (merged == null) { merged = resolved; - else + } else { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth() + 1, "merging " + merged + " with fallback " + resolved); merged = merged.withFallback(resolved); + } } count += 1; } + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth() + 1, "stack was merged to: " + merged); + return merged; } @@ -129,6 +152,8 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl List subStack = stack.subList(skipping, stack.size()); if (subStack.isEmpty()) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "Nothing else in the merge stack, can't resolve"); throw new NotPossibleToResolve(context); } else { // generate a new merge stack from only the remaining items diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java index 582c585c..d06b2c34 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -377,10 +377,12 @@ public class ConfigImpl { private static class DebugHolder { private static String LOADS = "loads"; + private static String SUBSTITUTIONS = "substitutions"; private static Map loadDiagnostics() { Map result = new HashMap(); result.put(LOADS, false); + result.put(SUBSTITUTIONS, false); // People do -Dconfig.trace=foo,bar to enable tracing of different things String s = System.getProperty("config.trace"); @@ -391,6 +393,8 @@ public class ConfigImpl { for (String k : keys) { if (k.equals(LOADS)) { result.put(LOADS, true); + } else if (k.equals(SUBSTITUTIONS)) { + result.put(SUBSTITUTIONS, true); } else { System.err.println("config.trace property contains unknown trace topic '" + k + "'"); @@ -403,10 +407,15 @@ public class ConfigImpl { private static final Map diagnostics = loadDiagnostics(); private static final boolean traceLoadsEnabled = diagnostics.get(LOADS); + private static final boolean traceSubstitutionsEnabled = diagnostics.get(SUBSTITUTIONS); static boolean traceLoadsEnabled() { return traceLoadsEnabled; } + + static boolean traceSubstitutionsEnabled() { + return traceSubstitutionsEnabled; + } } /** For use ONLY by library internals, DO NOT TOUCH not guaranteed ABI */ @@ -418,10 +427,26 @@ public class ConfigImpl { } } + public static boolean traceSubstitutionsEnabled() { + try { + return DebugHolder.traceSubstitutionsEnabled(); + } catch (ExceptionInInitializerError e) { + throw ConfigImplUtil.extractInitializerError(e); + } + } + public static void trace(String message) { System.err.println(message); } + public static void trace(int indentLevel, String message) { + while (indentLevel > 0) { + System.err.print(" "); + indentLevel -= 1; + } + System.err.println(message); + } + // the basic idea here is to add the "what" and have a canonical // toplevel error message. the "original" exception may however have extra // detail about what happened. call this if you have a better "what" than diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java index 8c024daf..204a0aba 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java @@ -72,6 +72,9 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable { try { v = context.source().lookupSubst(context, expr, prefixLength); } catch (NotPossibleToResolve e) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), + "not possible to resolve " + expr + ", cycle involved: " + e.traceString()); if (expr.optional()) v = null; else 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..c9c73e14 100644 --- a/config/src/main/java/com/typesafe/config/impl/MemoKey.java +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -36,4 +36,9 @@ final class MemoKey { return false; } } + + @Override + public final String toString() { + return "MemoKey(" + value + "," + restrictToChildOrNull + ")"; + } } 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 b73c064c..a6f9a469 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -43,6 +43,8 @@ final class ResolveContext { // in error messages if nothing else this(new ResolveSource(root), new ResolveMemos(), options, restrictToChild, new ArrayList()); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("ResolveContext at root " + root + " restrict to child " + restrictToChild); } ResolveSource source() { @@ -73,11 +75,15 @@ final class ResolveContext { } void trace(SubstitutionExpression expr) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "pushing expression " + expr); expressionTrace.add(expr); } void untrace() { - expressionTrace.remove(expressionTrace.size() - 1); + SubstitutionExpression expr = expressionTrace.remove(expressionTrace.size() - 1); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "popped expression " + expr); } String traceString() { @@ -92,7 +98,14 @@ final class ResolveContext { return sb.toString(); } + int depth() { + return expressionTrace.size(); + } + AbstractConfigValue resolve(AbstractConfigValue original) throws NotPossibleToResolve { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "resolving " + original); + // a fully-resolved (no restrictToChild) object can satisfy a // request for a restricted object, so always check that first. final MemoKey fullKey = new MemoKey(original, null); @@ -109,15 +122,26 @@ final class ResolveContext { } if (cached != null) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "using cached resolution " + cached + " for " + original); return cached; } else { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "not found in cache, resolving " + original); + AbstractConfigValue resolved = source.resolveCheckingReplacement(this, original); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "resolved to " + resolved + " from " + original); + 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. + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); + memos.put(fullKey, resolved); } else { // if we have an unresolved object then either we did a @@ -128,8 +152,14 @@ final class ResolveContext { throw new ConfigException.BugOrBroken( "restrictedKey should not be null here"); } + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "caching " + restrictedKey + " result " + resolved); + memos.put(restrictedKey, resolved); } else if (options().getAllowUnresolved()) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); + memos.put(fullKey, resolved); } else { throw new ConfigException.BugOrBroken( diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java index 1a5650f0..4cb1c364 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java @@ -24,6 +24,8 @@ abstract class ResolveReplacer { @Override protected AbstractConfigValue makeReplacement(ResolveContext context) throws NotPossibleToResolve { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "Cycle detected, can't resolve"); throw new NotPossibleToResolve(context); } }; diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index 6b5d5a18..9e150bad 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -31,8 +31,13 @@ final class ResolveSource { AbstractConfigValue lookupSubst(ResolveContext context, SubstitutionExpression subst, int prefixLength) throws NotPossibleToResolve { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "searching for " + subst); + context.trace(subst); try { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), subst + " - looking up relative to file it occurred in"); // First we look up the full path, which means relative to the // included file if we were not a root file AbstractConfigValue result = findInObject(root, context, subst); @@ -49,19 +54,30 @@ final class ResolveSource { context.trace(unprefixed); if (prefixLength > 0) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), unprefixed + " - looking up relative to parent file"); result = findInObject(root, context, unprefixed); } if (result == null && context.options().getUseSystemEnvironment()) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), unprefixed + " - looking up in system environment"); result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context, unprefixed); } } if (result != null) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "recursively resolving " + result + + " which was the resolution of " + subst); + result = context.resolve(result); } + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "resolved to " + result); + return result; } finally { context.untrace(); @@ -87,7 +103,12 @@ final class ResolveSource { if (replacer == null) { return value; } else { - return replacer.replace(context); + AbstractConfigValue replacement = replacer.replace(context); + if (ConfigImpl.traceSubstitutionsEnabled() && value != replacement) { + ConfigImpl.trace(" when looking up substitutions " + context.traceString() + " replaced " + value + + " with " + replacement); + } + return replacement; } } @@ -103,10 +124,15 @@ final class ResolveSource { if (replacement != original) { // start over, checking if replacement was memoized + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "for resolution, replaced " + original + " with " + replacement); return context.resolve(replacement); } else { AbstractConfigValue resolved; + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "resolving " + original + " with trace '" + context.traceString() + + "'"); resolved = original.resolveSubstitutions(context); return resolved; From 9e8532d3f0d3cf005cdc71703e692909d4a97a14 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 4 Jul 2014 22:16:25 -0400 Subject: [PATCH 02/14] Add useful toString to ResolveReplacer subtypes --- .../com/typesafe/config/impl/ConfigDelayedMerge.java | 5 +++++ .../typesafe/config/impl/ConfigDelayedMergeObject.java | 5 +++++ .../java/com/typesafe/config/impl/ResolveReplacer.java | 10 ++++++++++ 3 files changed, 20 insertions(+) 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 df942d5b..c84895cf 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -142,6 +142,11 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl throws NotPossibleToResolve { return ConfigDelayedMerge.makeReplacement(context, stack, skipping); } + + @Override + public String toString() { + return "ResolveReplacer(ConfigDelayedMerge substack skipping=" + skipping + ")"; + } }; } 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 7e6ced52..52b4cd88 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -69,6 +69,11 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm throws NotPossibleToResolve { return ConfigDelayedMerge.makeReplacement(context, stack, skipping); } + + @Override + public String toString() { + return "ResolveReplacer(ConfigDelayedMergeObject substack skipping=" + skipping + ")"; + } }; } diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java index 4cb1c364..b3c2549f 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java @@ -28,5 +28,15 @@ abstract class ResolveReplacer { ConfigImpl.trace(context.depth(), "Cycle detected, can't resolve"); throw new NotPossibleToResolve(context); } + + @Override + public String toString() { + return "ResolveReplacer(cycle detector)"; + } }; + + @Override + public String toString() { + return getClass().getSimpleName() + "(" + replacement + ")"; + } } From 7994e02c718fe780fd241f63b2759c629ef6b6bb Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 4 Jul 2014 22:35:29 -0400 Subject: [PATCH 03/14] Add identity hash codes to some substitution trace messages Otherwise you can't tell how it works since replacements and memoization are done by identity. --- .../src/main/java/com/typesafe/config/impl/MemoKey.java | 2 +- .../java/com/typesafe/config/impl/ResolveContext.java | 3 ++- .../java/com/typesafe/config/impl/ResolveSource.java | 9 +++++++-- 3 files changed, 10 insertions(+), 4 deletions(-) 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 c9c73e14..cd843dad 100644 --- a/config/src/main/java/com/typesafe/config/impl/MemoKey.java +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -39,6 +39,6 @@ final class MemoKey { @Override public final String toString() { - return "MemoKey(" + value + "," + restrictToChildOrNull + ")"; + return "MemoKey(" + value + "@" + System.identityHashCode(value) + "," + restrictToChildOrNull + ")"; } } 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 a6f9a469..42e54d72 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -127,7 +127,8 @@ final class ResolveContext { return cached; } else { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "not found in cache, resolving " + original); + ConfigImpl.trace(depth(), + "not found in cache, resolving " + original + "@" + System.identityHashCode(original)); AbstractConfigValue resolved = source.resolveCheckingReplacement(this, original); diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index 9e150bad..65fefccd 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -85,6 +85,8 @@ final class ResolveSource { } void replace(AbstractConfigValue value, ResolveReplacer replacer) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("Replacing " + value + "@" + System.identityHashCode(value) + " with " + replacer); ResolveReplacer old = replacements.put(value, replacer); if (old != null) throw new ConfigException.BugOrBroken("should not have replaced the same value twice: " @@ -95,6 +97,9 @@ final class ResolveSource { ResolveReplacer replacer = replacements.remove(value); if (replacer == null) throw new ConfigException.BugOrBroken("unreplace() without replace(): " + value); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("Unreplacing " + value + "@" + System.identityHashCode(value) + " which was replaced by " + + replacer); } private AbstractConfigValue replacement(ResolveContext context, AbstractConfigValue value) @@ -131,8 +136,8 @@ final class ResolveSource { AbstractConfigValue resolved; if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "resolving " + original + " with trace '" + context.traceString() - + "'"); + ConfigImpl.trace(context.depth(), + "resolving unreplaced " + original + " with trace '" + context.traceString() + "'"); resolved = original.resolveSubstitutions(context); return resolved; From 66d21576b1c9aec7b9b021a7275a178816218420 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 4 Jul 2014 23:44:54 -0400 Subject: [PATCH 04/14] Track parent values as we resolve In this patch, it's just to indent the debug output nicely. --- .../typesafe/config/impl/ResolveContext.java | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) 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 42e54d72..2f17c0b8 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -29,20 +29,24 @@ final class ResolveContext { // resolution fails. final private List expressionTrace; - ResolveContext(ResolveSource source, ResolveMemos memos, ConfigResolveOptions options, - Path restrictToChild, List expressionTrace) { + // This is used for tracing and debugging. + final private List treeStack; + + ResolveContext(ResolveSource source, ResolveMemos memos, ConfigResolveOptions options, Path restrictToChild, + List expressionTrace, List treeStack) { this.source = source; this.memos = memos; this.options = options; this.restrictToChild = restrictToChild; this.expressionTrace = expressionTrace; + this.treeStack = treeStack; } ResolveContext(AbstractConfigObject root, ConfigResolveOptions options, Path restrictToChild) { // LinkedHashSet keeps the traversal order which is at least useful // in error messages if nothing else this(new ResolveSource(root), new ResolveMemos(), options, restrictToChild, - new ArrayList()); + new ArrayList(), new ArrayList()); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace("ResolveContext at root " + root + " restrict to child " + restrictToChild); } @@ -67,7 +71,7 @@ final class ResolveContext { if (restrictTo == restrictToChild) return this; else - return new ResolveContext(source, memos, options, restrictTo, expressionTrace); + return new ResolveContext(source, memos, options, restrictTo, expressionTrace, treeStack); } ResolveContext unrestricted() { @@ -98,14 +102,32 @@ final class ResolveContext { return sb.toString(); } + void stack(AbstractConfigValue value) { + treeStack.add(value); + } + + void unstack() { + treeStack.remove(treeStack.size() - 1); + } + int depth() { - return expressionTrace.size(); + return treeStack.size(); } AbstractConfigValue resolve(AbstractConfigValue original) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "resolving " + original); + AbstractConfigValue resolved; + stack(original); + try { + resolved = realResolve(original); + } finally { + unstack(); + } + return resolved; + } + private AbstractConfigValue realResolve(AbstractConfigValue original) throws NotPossibleToResolve { // a fully-resolved (no restrictToChild) object can satisfy a // request for a restricted object, so always check that first. final MemoKey fullKey = new MemoKey(original, null); From fdce50fb76d9ca9dacb34108c4499af188f88421 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Mon, 7 Jul 2014 21:19:09 -0400 Subject: [PATCH 05/14] Add shortcut that identity-equal lists/objects are equals() The intent here is to make it fast to compare something to itself. Though maybe Java does this automatically, not sure. --- .../java/com/typesafe/config/impl/ConfigDelayedMerge.java | 3 ++- .../com/typesafe/config/impl/ConfigDelayedMergeObject.java | 4 ++-- .../main/java/com/typesafe/config/impl/SimpleConfigList.java | 3 ++- .../java/com/typesafe/config/impl/SimpleConfigObject.java | 3 +++ 4 files changed, 9 insertions(+), 4 deletions(-) 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 c84895cf..206f18b0 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -233,7 +233,8 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // note that "origin" is deliberately NOT part of equality if (other instanceof ConfigDelayedMerge) { return canEqual(other) - && this.stack.equals(((ConfigDelayedMerge) other).stack); + && (this.stack == ((ConfigDelayedMerge) other).stack || this.stack + .equals(((ConfigDelayedMerge) other).stack)); } else { return false; } 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 52b4cd88..6b11f108 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -170,8 +170,8 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm // note that "origin" is deliberately NOT part of equality if (other instanceof ConfigDelayedMergeObject) { return canEqual(other) - && this.stack - .equals(((ConfigDelayedMergeObject) other).stack); + && (this.stack == ((ConfigDelayedMergeObject) other).stack || this.stack + .equals(((ConfigDelayedMergeObject) other).stack)); } else { return false; } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index 5ec13f29..d197cd25 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -154,7 +154,8 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList, // note that "origin" is deliberately NOT part of equality if (other instanceof SimpleConfigList) { // optimization to avoid unwrapped() for two ConfigList - return canEqual(other) && value.equals(((SimpleConfigList) other).value); + return canEqual(other) + && (value == ((SimpleConfigList) other).value || value.equals(((SimpleConfigList) other).value)); } else { return false; } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java index 5cbf1030..7667a20f 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -443,6 +443,9 @@ final class SimpleConfigObject extends AbstractConfigObject implements Serializa } private static boolean mapEquals(Map a, Map b) { + if (a == b) + return true; + Set aKeys = a.keySet(); Set bKeys = b.keySet(); From 0a20b9ad73da7d9359bfab511db710a18072bd65 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Thu, 10 Jul 2014 12:58:58 -0400 Subject: [PATCH 06/14] Rewrite substitution resolver, use explicit immutable ResolveSource The immediate motivation here was to fix #177, which this does, but in this commit a couple of existing test cases are broken in a way which seems to relate to order of resolution and resolve memoization. So we need to layer on to this commit better solutions for caching and cycle detection to get rid of yet more mutable state. The previous setup used a side-effect-based lookup table of "replacement" values to conceptually modify the tree without actually modifying it. Unfortunately that setup was hacky and hard to reason about and, apparently, broken in cases such as #177. This new setup actually creates a modified tree and passes it around explicitly instead of inside ResolveContext. In this commit, ResolveContext still unfortunately has a mutable cache and a mutable table of "cycle markers." Both of those in theory could also be replaced by simply modifying the tree. The main downside to this commit - and to cleaning up the remaining mutable state - is that we're using Java collections which have to be copied wholesale for every mutation (they are not persistent functional data structures). This will have an unknown performance impact, though in a sane world Config.resolve() is not a bottleneck in anyone's production app. Some other details of this commit: * resolve concerns removed from peekPath in AbstractConfigObject and relocated into ResolveSource * recursive resolution removed from lookupSubst and moved to ConfigReference * new hasDescendant() method used only in debug tracing, it is grossly inefficient to ever call this full tree traversal * new replaceChild() method is inefficient due to Java collections but could in theory be made efficient * most complexity relates to always knowing the parent of a node that we might have to replace, so we can walk up replacing it in its ancestor chain TODO in subsequent commits: * fix failing test cases * we cannot replaceChild if we are a descendant of ConfigConcatenation, but we probably (?) need to be able to; consider / fix this * instead of memoizing resolve results in a hash table, just continuously modify the ResolveSource to have the most recent results * instead of using the "cycle markers" table, change the ConfigReference to a cycle detector value --- config/.gitignore | 1 + .../config/impl/AbstractConfigObject.java | 77 +-- .../config/impl/AbstractConfigValue.java | 36 +- .../config/impl/ConfigConcatenation.java | 25 +- .../config/impl/ConfigDelayedMerge.java | 139 +++--- .../config/impl/ConfigDelayedMergeObject.java | 34 +- .../typesafe/config/impl/ConfigReference.java | 27 +- .../com/typesafe/config/impl/Container.java | 25 + .../com/typesafe/config/impl/MemoKey.java | 7 +- .../config/impl/ReplaceableMergeStack.java | 8 +- .../typesafe/config/impl/ResolveContext.java | 124 ++--- .../typesafe/config/impl/ResolveReplacer.java | 42 -- .../typesafe/config/impl/ResolveSource.java | 444 ++++++++++++++---- .../config/impl/SimpleConfigList.java | 25 +- .../config/impl/SimpleConfigObject.java | 42 +- .../config/impl/ConcatenationTest.scala | 34 +- .../config/impl/ConfigSubstitutionTest.scala | 16 +- 17 files changed, 753 insertions(+), 353 deletions(-) create mode 100644 config/.gitignore create mode 100644 config/src/main/java/com/typesafe/config/impl/Container.java delete mode 100644 config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java diff --git a/config/.gitignore b/config/.gitignore new file mode 100644 index 00000000..5e56e040 --- /dev/null +++ b/config/.gitignore @@ -0,0 +1 @@ +/bin diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index 394931de..bcda80d7 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -17,7 +17,7 @@ import com.typesafe.config.ConfigRenderOptions; import com.typesafe.config.ConfigValue; import com.typesafe.config.ConfigValueType; -abstract class AbstractConfigObject extends AbstractConfigValue implements ConfigObject { +abstract class AbstractConfigObject extends AbstractConfigValue implements ConfigObject, Container { final private SimpleConfig config; @@ -56,7 +56,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Confi /** * This looks up the key with no transformation or type conversion of any * kind, and returns null if the key is not present. The object must be - * resolved; use attemptPeekWithPartialResolve() if it is not. + * resolved along the nodes needed to get the key or + * ConfigException.NotResolved will be thrown. * * @param key * @return the unmodified raw value or null @@ -78,67 +79,34 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Confi * key to look up * @return the value of the key, or null if known not to exist * @throws ConfigException.NotResolved - * if can't figure out key's value or can't know whether it - * exists + * if can't figure out key's value (or existence) without more + * resolving */ - protected abstract AbstractConfigValue attemptPeekWithPartialResolve(String key); + abstract AbstractConfigValue attemptPeekWithPartialResolve(String key); /** - * Looks up the path with no transformation, type conversion, or exceptions - * (just returns null if path not found). Does however resolve the path, if - * resolver != null. - * - * @throws NotPossibleToResolve - * if context is not null and resolution fails + * Looks up the path with no transformation or type conversion. Returns null + * if the path is not found; throws ConfigException.NotResolved if we need + * to go through an unresolved node to look up the path. */ - protected AbstractConfigValue peekPath(Path path, ResolveContext context) throws NotPossibleToResolve { - return peekPath(this, path, context); + protected AbstractConfigValue peekPath(Path path) { + return peekPath(this, path); } - /** - * Looks up the path. Doesn't do any resolution, will throw if any is - * needed. - */ - AbstractConfigValue peekPath(Path path) { + private static AbstractConfigValue peekPath(AbstractConfigObject self, Path path) { try { - return peekPath(this, path, null); - } catch (NotPossibleToResolve e) { - throw new ConfigException.BugOrBroken( - "NotPossibleToResolve happened though we had no ResolveContext in peekPath"); - } - } + // we'll fail if anything along the path can't + // be looked at without resolving. + Path next = path.remainder(); + AbstractConfigValue v = self.attemptPeekWithPartialResolve(path.first()); - // as a side effect, peekPath() will have to resolve all parents of the - // child being peeked, but NOT the child itself. Caller has to resolve - // the child itself if needed. - private static AbstractConfigValue peekPath(AbstractConfigObject self, Path path, - ResolveContext context) throws NotPossibleToResolve { - try { - if (context != null) { - // walk down through the path resolving only things along that - // path, and then recursively call ourselves with no resolve - // context. - AbstractConfigValue partiallyResolved = context.restrict(path).resolve(self); - if (partiallyResolved instanceof AbstractConfigObject) { - return peekPath((AbstractConfigObject) partiallyResolved, path, null); - } else { - throw new ConfigException.BugOrBroken("resolved object to non-object " + self - + " to " + partiallyResolved); - } + if (next == null) { + return v; } else { - // with no resolver, we'll fail if anything along the path can't - // be looked at without resolving. - Path next = path.remainder(); - AbstractConfigValue v = self.attemptPeekWithPartialResolve(path.first()); - - if (next == null) { - return v; + if (v instanceof AbstractConfigObject) { + return peekPath((AbstractConfigObject) v, next); } else { - if (v instanceof AbstractConfigObject) { - return peekPath((AbstractConfigObject) v, next, null); - } else { - return null; - } + return null; } } } catch (ConfigException.NotResolved e) { @@ -209,7 +177,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Confi } @Override - abstract AbstractConfigObject resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve; + abstract AbstractConfigObject resolveSubstitutions(ResolveContext context, ResolveSource source) + throws NotPossibleToResolve; @Override abstract AbstractConfigObject relativized(final Path prefix); diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index 9b6b1fd9..1eb05c81 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -68,9 +68,11 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue { * * @param context * state of the current resolve + * @param source + * where to look up values * @return a new value if there were changes, or this if no changes */ - AbstractConfigValue resolveSubstitutions(ResolveContext context) + AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { return this; } @@ -79,6 +81,38 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue { return ResolveStatus.RESOLVED; } + protected static List replaceChildInList(List list, + AbstractConfigValue child, AbstractConfigValue replacement) { + int i = 0; + while (i < list.size() && list.get(i) != child) + ++i; + if (i == list.size()) + throw new ConfigException.BugOrBroken("tried to replace " + child + " which is not in " + list); + List newStack = new ArrayList(list); + if (replacement != null) + newStack.set(i, replacement); + else + newStack.remove(i); + + if (newStack.isEmpty()) + return null; + else + return newStack; + } + + protected static boolean hasDescendantInList(List list, AbstractConfigValue descendant) { + for (AbstractConfigValue v : list) { + if (v == descendant) + return true; + } + // now the expensive traversal + for (AbstractConfigValue v : list) { + if (v instanceof Container && ((Container) v).hasDescendant(descendant)) + return true; + } + return false; + } + /** * This is used when including one file in another; the included file is * relativized to the path it's included into in the parent file. The point diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java index 8cb15753..e4f031eb 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java @@ -22,7 +22,7 @@ import com.typesafe.config.ConfigValueType; * concatenations of objects, but ConfigDelayedMerge should be used for that * since a concat of objects really will merge, not concatenate. */ -final class ConfigConcatenation extends AbstractConfigValue implements Unmergeable { +final class ConfigConcatenation extends AbstractConfigValue implements Unmergeable, Container { final private List pieces; @@ -170,7 +170,7 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab } @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve { + AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) { int indent = context.depth() + 2; ConfigImpl.trace(indent - 1, "concatenation has " + pieces.size() + " pieces:"); @@ -181,11 +181,16 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab } } + // Right now there's no reason to pushParent here because the + // content of ConfigConcatenation should not need to replaceChild, + // but if it did we'd have to do this. + ResolveSource sourceWithParent = source; // .pushParent(this); + List resolved = new ArrayList(pieces.size()); for (AbstractConfigValue p : pieces) { // to concat into a string we have to do a full resolve, // so unrestrict the context - AbstractConfigValue r = context.unrestricted().resolve(p); + AbstractConfigValue r = context.unrestricted().resolve(p, sourceWithParent); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(context.depth(), "resolved concat piece to " + r); if (r == null) { @@ -215,6 +220,20 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab return ResolveStatus.UNRESOLVED; } + @Override + public ConfigConcatenation replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) { + List newPieces = replaceChildInList(pieces, child, replacement); + if (newPieces == null) + return null; + else + return new ConfigConcatenation(origin(), newPieces); + } + + @Override + public boolean hasDescendant(AbstractConfigValue descendant) { + return hasDescendantInList(pieces, descendant); + } + // when you graft a substitution into another object, // you have to prefix it with the location in that object // where you grafted it; but save prefixLength so 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 206f18b0..32a8ea65 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -54,20 +54,19 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl } @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context) + AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { - return resolveSubstitutions(this, stack, context); + return resolveSubstitutions(this, stack, context, source); } // static method also used by ConfigDelayedMergeObject - static AbstractConfigValue resolveSubstitutions(ReplaceableMergeStack replaceable, - List stack, ResolveContext context) throws NotPossibleToResolve { + static AbstractConfigValue resolveSubstitutions(ReplaceableMergeStack replaceable, List stack, + ResolveContext context, ResolveSource source) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) { - int indent = context.depth() + 2; - ConfigImpl.trace(indent - 1, "delayed merge stack has " + stack.size() + " items:"); + ConfigImpl.trace(context.depth(), "delayed merge stack has " + stack.size() + " items:"); int count = 0; for (AbstractConfigValue v : stack) { - ConfigImpl.trace(indent, count + ": " + v); + ConfigImpl.trace(context.depth() + 1, count + ": " + v); count += 1; } } @@ -75,91 +74,93 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // 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. + // is non-null, or resolve options allow partial resolves, + // we may remain a delayed merge though. int count = 0; AbstractConfigValue merged = null; - for (AbstractConfigValue v : stack) { - if (v instanceof ReplaceableMergeStack) - throw new ConfigException.BugOrBroken( - "A delayed merge should not contain another one: " + replaceable); + for (AbstractConfigValue end : stack) { + // the end value may or may not be resolved already - boolean replaced = false; - // we only replace if we have a substitution, or - // value-concatenation containing one. The Unmergeable - // here isn't a delayed merge stack since we can't contain - // another stack (see assertion above). - if (v instanceof Unmergeable) { - // If, while resolving 'v' we come back to the same - // merge stack, we only want to look _below_ 'v' + ResolveSource sourceForEnd; + + if (end instanceof ReplaceableMergeStack) + throw new ConfigException.BugOrBroken("A delayed merge should not contain another one: " + replaceable); + else if (end instanceof Unmergeable) { + // the remainder could be any kind of value, including another + // ConfigDelayedMerge + AbstractConfigValue remainder = replaceable.makeReplacement(context, count + 1); + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "remainder portion: " + remainder); + + // If, while resolving 'end' we come back to the same + // merge stack, we only want to look _below_ 'end' // in the stack. So we arrange to replace the // ConfigDelayedMerge with a value that is only // the remainder of the stack below this one. if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth() + 1, "because item " + count - + " in this stack is unresolved, resolving it can only look at remaining " - + (stack.size() - count - 1) + " items"); - context.source().replace((AbstractConfigValue) replaceable, - replaceable.makeReplacer(count + 1)); - replaced = true; - } + ConfigImpl.trace(context.depth(), "building sourceForEnd"); + + // we resetParents() here because we'll be resolving "end" + // against a root which does NOT contain "end" + sourceForEnd = source.replaceWithinCurrentParent((AbstractConfigValue) replaceable, remainder); - AbstractConfigValue resolved; - try { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth() + 1, "resolving item " + count + " in merge stack of " - + stack.size()); - resolved = context.resolve(v); - } finally { - if (replaced) - context.source().unreplace((AbstractConfigValue) replaceable); + ConfigImpl.trace(context.depth(), " sourceForEnd before reset parents but after replace: " + + sourceForEnd); + + sourceForEnd = sourceForEnd.resetParents(); + } else { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl + .trace(context.depth(), "will resolve end against the original source with parent pushed"); + + sourceForEnd = source.pushParent(replaceable); } - if (resolved != null) { + if (ConfigImpl.traceSubstitutionsEnabled()) { + ConfigImpl.trace(context.depth(), "sourceForEnd =" + sourceForEnd); + } + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "Resolving highest-priority item in delayed merge " + end + + " against " + sourceForEnd + " endWasRemoved=" + (source != sourceForEnd)); + AbstractConfigValue resolvedEnd = context.resolve(end, sourceForEnd); + + if (resolvedEnd != null) { if (merged == null) { - merged = resolved; + merged = resolvedEnd; } else { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth() + 1, "merging " + merged + " with fallback " + resolved); - merged = merged.withFallback(resolved); + ConfigImpl.trace(context.depth() + 1, "merging " + merged + " with fallback " + resolvedEnd); + merged = merged.withFallback(resolvedEnd); } } - count += 1; - } - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth() + 1, "stack was merged to: " + merged); + count += 1; + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "stack merged, yielding: " + merged); + } return merged; } @Override - public ResolveReplacer makeReplacer(final int skipping) { - return new ResolveReplacer() { - @Override - protected AbstractConfigValue makeReplacement(ResolveContext context) - throws NotPossibleToResolve { - return ConfigDelayedMerge.makeReplacement(context, stack, skipping); - } - - @Override - public String toString() { - return "ResolveReplacer(ConfigDelayedMerge substack skipping=" + skipping + ")"; - } - }; + public AbstractConfigValue makeReplacement(ResolveContext context, int skipping) { + return ConfigDelayedMerge.makeReplacement(context, stack, skipping); } - // static method also used by ConfigDelayedMergeObject - static AbstractConfigValue makeReplacement(ResolveContext context, - List stack, int skipping) throws NotPossibleToResolve { - + // static method also used by ConfigDelayedMergeObject; end may be null + static AbstractConfigValue makeReplacement(ResolveContext context, List stack, int skipping) { List subStack = stack.subList(skipping, stack.size()); if (subStack.isEmpty()) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "Nothing else in the merge stack, can't resolve"); - throw new NotPossibleToResolve(context); + ConfigImpl.trace(context.depth(), "Nothing else in the merge stack, replacing with null"); + return null; } else { // generate a new merge stack from only the remaining items AbstractConfigValue merged = null; @@ -178,6 +179,20 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl return ResolveStatus.UNRESOLVED; } + @Override + public AbstractConfigValue replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) { + List newStack = replaceChildInList(stack, child, replacement); + if (newStack == null) + return null; + else + return new ConfigDelayedMerge(origin(), newStack); + } + + @Override + public boolean hasDescendant(AbstractConfigValue descendant) { + return hasDescendantInList(stack, descendant); + } + @Override ConfigDelayedMerge relativized(Path prefix) { List newStack = new ArrayList(); 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 6b11f108..f9afd6d7 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -50,9 +50,8 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm } @Override - AbstractConfigObject resolveSubstitutions(ResolveContext context) - throws NotPossibleToResolve { - AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(this, stack, context); + AbstractConfigObject resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { + AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(this, stack, context, source); if (merged instanceof AbstractConfigObject) { return (AbstractConfigObject) merged; } else { @@ -62,19 +61,8 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm } @Override - public ResolveReplacer makeReplacer(final int skipping) { - return new ResolveReplacer() { - @Override - protected AbstractConfigValue makeReplacement(ResolveContext context) - throws NotPossibleToResolve { - return ConfigDelayedMerge.makeReplacement(context, stack, skipping); - } - - @Override - public String toString() { - return "ResolveReplacer(ConfigDelayedMergeObject substack skipping=" + skipping + ")"; - } - }; + public AbstractConfigValue makeReplacement(ResolveContext context, int skipping) { + return ConfigDelayedMerge.makeReplacement(context, stack, skipping); } @Override @@ -82,6 +70,20 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm return ResolveStatus.UNRESOLVED; } + @Override + public AbstractConfigValue replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) { + List newStack = replaceChildInList(stack, child, replacement); + if (newStack == null) + return null; + else + return new ConfigDelayedMergeObject(origin(), newStack); + } + + @Override + public boolean hasDescendant(AbstractConfigValue descendant) { + return hasDescendantInList(stack, descendant); + } + @Override ConfigDelayedMergeObject relativized(Path prefix) { List newStack = new ArrayList(); diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java index 204a0aba..963157cb 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java @@ -65,12 +65,28 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable { // This way it's impossible for NotPossibleToResolve to "escape" since // any failure to resolve has to start with a ConfigReference. @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context) { - context.source().replace(this, ResolveReplacer.cycleResolveReplacer); + AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) { + context.addCycleMarker(this); try { AbstractConfigValue v; try { - v = context.source().lookupSubst(context, expr, prefixLength); + ResolveSource.ValueWithPath valueWithPath = source.lookupSubst(context, expr, prefixLength); + + if (valueWithPath.value != null) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "recursively resolving " + valueWithPath + + " which was the resolution of " + expr + " against " + source); + + ResolveSource recursiveResolveSource = (new ResolveSource( + (AbstractConfigObject) valueWithPath.pathFromRoot.last(), valueWithPath.pathFromRoot)); + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "will recursively resolve against " + recursiveResolveSource); + + v = context.resolve(valueWithPath.value, recursiveResolveSource); + } else { + v = null; + } } catch (NotPossibleToResolve e) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(context.depth(), @@ -79,8 +95,7 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable { v = null; else throw new ConfigException.UnresolvedSubstitution(origin(), expr - + " was part of a cycle of substitutions involving " + e.traceString(), - e); + + " was part of a cycle of substitutions involving " + e.traceString(), e); } if (v == null && !expr.optional()) { @@ -92,7 +107,7 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable { return v; } } finally { - context.source().unreplace(this); + context.removeCycleMarker(this); } } diff --git a/config/src/main/java/com/typesafe/config/impl/Container.java b/config/src/main/java/com/typesafe/config/impl/Container.java new file mode 100644 index 00000000..73a40391 --- /dev/null +++ b/config/src/main/java/com/typesafe/config/impl/Container.java @@ -0,0 +1,25 @@ +/** + * Copyright (C) 2014 Typesafe Inc. + */ +package com.typesafe.config.impl; + +/** + * An AbstractConfigValue which contains other values. Java has no way to + * express "this has to be an AbstractConfigValue also" other than making + * AbstractConfigValue an interface which would be aggravating. But we can say + * we are a ConfigValue. + */ +interface Container extends com.typesafe.config.ConfigValue { + /** + * Replace a child of this value. CAUTION if replacement is null, delete the + * child, which may also delete the parent, or make the parent into a + * non-container. + */ + AbstractConfigValue replaceChild(AbstractConfigValue child, AbstractConfigValue replacement); + + /** + * Super-expensive full traversal to see if descendant is anywhere + * underneath this container. + */ + boolean hasDescendant(AbstractConfigValue descendant); +} 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 cd843dad..d948aed0 100644 --- a/config/src/main/java/com/typesafe/config/impl/MemoKey.java +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -2,17 +2,20 @@ package com.typesafe.config.impl; /** The key used to memoize already-traversed nodes when resolving substitutions */ final class MemoKey { - MemoKey(AbstractConfigValue value, Path restrictToChildOrNull) { + MemoKey(AbstractConfigValue root, AbstractConfigValue value, Path restrictToChildOrNull) { + this.root = root; this.value = value; this.restrictToChildOrNull = restrictToChildOrNull; } + final private AbstractConfigValue root; final private AbstractConfigValue value; final private Path restrictToChildOrNull; @Override public final int hashCode() { int h = System.identityHashCode(value); + h = h + 41 * (41 + root.hashCode()); if (restrictToChildOrNull != null) { return h + 41 * (41 + restrictToChildOrNull.hashCode()); } else { @@ -26,6 +29,8 @@ final class MemoKey { MemoKey o = (MemoKey) other; if (o.value != this.value) return false; + else if (o.root != this.root) + return false; else if (o.restrictToChildOrNull == this.restrictToChildOrNull) return true; else if (o.restrictToChildOrNull == null || this.restrictToChildOrNull == null) diff --git a/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java b/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java index 7e5dc9d1..8425a5a3 100644 --- a/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java +++ b/config/src/main/java/com/typesafe/config/impl/ReplaceableMergeStack.java @@ -5,10 +5,10 @@ package com.typesafe.config.impl; * that replaces itself during substitution resolution in order to implement * "look backwards only" semantics. */ -interface ReplaceableMergeStack { +interface ReplaceableMergeStack extends Container { /** - * Make a replacer for this object, skipping the given number of items in - * the stack. + * Make a replacement for this object skipping the given number of elements + * which are lower in merge priority. */ - ResolveReplacer makeReplacer(int skipping); + AbstractConfigValue makeReplacement(ResolveContext context, 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 2f17c0b8..49b1ec22 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -1,17 +1,16 @@ package com.typesafe.config.impl; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.ArrayList; +import java.util.Set; import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigResolveOptions; import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; final class ResolveContext { - // this is unfortunately mutable so should only be shared among - // ResolveContext in the same traversal. - final private ResolveSource source; - // this is unfortunately mutable so should only be shared among // ResolveContext in the same traversal. final private ResolveMemos memos; @@ -24,35 +23,42 @@ final class ResolveContext { // CAN BE NULL for a full resolve. final private Path restrictToChild; - // another mutable unfortunate. This is - // used to make nice error messages when - // resolution fails. - final private List expressionTrace; + // This is used for tracing and debugging and nice error messages; + // contains every node as we call resolve on it. + final private List resolveStack; - // This is used for tracing and debugging. - final private List treeStack; + final private Set cycleMarkers; - ResolveContext(ResolveSource source, ResolveMemos memos, ConfigResolveOptions options, Path restrictToChild, - List expressionTrace, List treeStack) { - this.source = source; + ResolveContext(ResolveMemos memos, ConfigResolveOptions options, Path restrictToChild, + List resolveStack, Set cycleMarkers) { this.memos = memos; this.options = options; this.restrictToChild = restrictToChild; - this.expressionTrace = expressionTrace; - this.treeStack = treeStack; + this.resolveStack = resolveStack; + this.cycleMarkers = cycleMarkers; } - ResolveContext(AbstractConfigObject root, ConfigResolveOptions options, Path restrictToChild) { + ResolveContext(ConfigResolveOptions options, Path restrictToChild) { // LinkedHashSet keeps the traversal order which is at least useful // in error messages if nothing else - this(new ResolveSource(root), new ResolveMemos(), options, restrictToChild, - new ArrayList(), new ArrayList()); + this(new ResolveMemos(), options, restrictToChild, new ArrayList(), Collections + .newSetFromMap(new IdentityHashMap())); if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace("ResolveContext at root " + root + " restrict to child " + restrictToChild); + ConfigImpl.trace(depth(), "ResolveContext restrict to child " + restrictToChild); } - ResolveSource source() { - return source; + void addCycleMarker(AbstractConfigValue value) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "++ Cycle marker " + value + "@" + System.identityHashCode(value)); + if (cycleMarkers.contains(value)) + throw new ConfigException.BugOrBroken("Added cycle marker twice " + value); + cycleMarkers.add(value); + } + + void removeCycleMarker(AbstractConfigValue value) { + cycleMarkers.remove(value); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "-- Cycle marker " + value + "@" + System.identityHashCode(value)); } ConfigResolveOptions options() { @@ -71,66 +77,64 @@ final class ResolveContext { if (restrictTo == restrictToChild) return this; else - return new ResolveContext(source, memos, options, restrictTo, expressionTrace, treeStack); + return new ResolveContext(memos, options, restrictTo, resolveStack, cycleMarkers); } ResolveContext unrestricted() { return restrict(null); } - void trace(SubstitutionExpression expr) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "pushing expression " + expr); - expressionTrace.add(expr); - } - - void untrace() { - SubstitutionExpression expr = expressionTrace.remove(expressionTrace.size() - 1); - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "popped expression " + expr); - } - String traceString() { String separator = ", "; StringBuilder sb = new StringBuilder(); - for (SubstitutionExpression expr : expressionTrace) { - sb.append(expr.toString()); - sb.append(separator); + for (AbstractConfigValue value : resolveStack) { + if (value instanceof ConfigReference) { + sb.append(((ConfigReference) value).expression().toString()); + sb.append(separator); + } } if (sb.length() > 0) sb.setLength(sb.length() - separator.length()); return sb.toString(); } - void stack(AbstractConfigValue value) { - treeStack.add(value); + private void pushTrace(AbstractConfigValue value) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "pushing trace " + value); + resolveStack.add(value); } - void unstack() { - treeStack.remove(treeStack.size() - 1); + private void popTrace() { + AbstractConfigValue old = resolveStack.remove(resolveStack.size() - 1); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), "popped trace " + old); } int depth() { - return treeStack.size(); + if (resolveStack.size() > 30) + throw new ConfigException.BugOrBroken("resolve getting too deep"); + return resolveStack.size(); } - AbstractConfigValue resolve(AbstractConfigValue original) throws NotPossibleToResolve { + AbstractConfigValue resolve(AbstractConfigValue original, ResolveSource source) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "resolving " + original); + ConfigImpl + .trace(depth(), "resolving " + original + " restrictToChild=" + restrictToChild + " in " + source); AbstractConfigValue resolved; - stack(original); + pushTrace(original); try { - resolved = realResolve(original); + resolved = realResolve(original, source); } finally { - unstack(); + popTrace(); } return resolved; } - private AbstractConfigValue realResolve(AbstractConfigValue original) throws NotPossibleToResolve { + private AbstractConfigValue realResolve(AbstractConfigValue original, ResolveSource source) + throws NotPossibleToResolve { // a fully-resolved (no restrictToChild) object can satisfy a // request for a restricted object, so always check that first. - final MemoKey fullKey = new MemoKey(original, null); + final MemoKey fullKey = new MemoKey(source.root, original, null); MemoKey restrictedKey = null; AbstractConfigValue cached = memos.get(fullKey); @@ -139,23 +143,32 @@ final class ResolveContext { // compute the restrictToChild object so use a more limited // memo key if (cached == null && isRestrictedToChild()) { - restrictedKey = new MemoKey(original, restrictToChild()); + restrictedKey = new MemoKey(source.root, original, restrictToChild()); cached = memos.get(restrictedKey); } if (cached != null) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "using cached resolution " + cached + " for " + original); + ConfigImpl.trace(depth(), "using cached resolution " + cached + " for " + original + + " restrictToChild " + restrictToChild()); return cached; } else { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "not found in cache, resolving " + original + "@" + System.identityHashCode(original)); - AbstractConfigValue resolved = source.resolveCheckingReplacement(this, original); + if (cycleMarkers.contains(original)) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(depth(), + "Cycle detected, can't resolve; " + original + "@" + System.identityHashCode(original)); + throw new NotPossibleToResolve(this); + } + + AbstractConfigValue resolved = original.resolveSubstitutions(this, source); if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "resolved to " + resolved + " from " + original); + ConfigImpl.trace(depth(), "resolved to " + resolved + "@" + System.identityHashCode(resolved) + + " from " + original + "@" + System.identityHashCode(resolved)); if (resolved == null || resolved.resolveStatus() == ResolveStatus.RESOLVED) { // if the resolved object is fully resolved by resolving @@ -196,10 +209,11 @@ final class ResolveContext { static AbstractConfigValue resolve(AbstractConfigValue value, AbstractConfigObject root, ConfigResolveOptions options) { - ResolveContext context = new ResolveContext(root, options, null /* restrictToChild */); + ResolveSource source = new ResolveSource(root); + ResolveContext context = new ResolveContext(options, null /* restrictToChild */); try { - return context.resolve(value); + return context.resolve(value, source); } catch (NotPossibleToResolve e) { // ConfigReference was supposed to catch NotPossibleToResolve throw new ConfigException.BugOrBroken( diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java b/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java deleted file mode 100644 index b3c2549f..00000000 --- a/config/src/main/java/com/typesafe/config/impl/ResolveReplacer.java +++ /dev/null @@ -1,42 +0,0 @@ -package com.typesafe.config.impl; - -import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; - -/** Callback that generates a replacement to use for resolving a substitution. */ -abstract class ResolveReplacer { - // 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(ResolveContext context) throws NotPossibleToResolve { - if (replacement == null) - replacement = makeReplacement(context); - return replacement; - } - - protected abstract AbstractConfigValue makeReplacement(ResolveContext context) - throws NotPossibleToResolve; - - static final ResolveReplacer cycleResolveReplacer = new ResolveReplacer() { - @Override - protected AbstractConfigValue makeReplacement(ResolveContext context) - throws NotPossibleToResolve { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "Cycle detected, can't resolve"); - throw new NotPossibleToResolve(context); - } - - @Override - public String toString() { - return "ResolveReplacer(cycle detector)"; - } - }; - - @Override - public String toString() { - return getClass().getSimpleName() + "(" + replacement + ")"; - } -} diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index 65fefccd..514c38a7 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -1,7 +1,7 @@ package com.typesafe.config.impl; -import java.util.IdentityHashMap; -import java.util.Map; +import java.util.Iterator; +import java.util.NoSuchElementException; import com.typesafe.config.ConfigException; import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; @@ -10,137 +10,385 @@ import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; * This class is the source for values for a substitution like ${foo}. */ final class ResolveSource { - final private AbstractConfigObject root; - // Conceptually, we transform the ResolveSource whenever we traverse - // a substitution or delayed merge stack, in order to remove the - // traversed node and therefore avoid circular dependencies. - // We implement it with this somewhat hacky "patch a replacement" - // mechanism instead of actually transforming the tree. - final private Map replacements; + + final AbstractConfigObject root; + // This is used for knowing the chain of parents we used to get here. + // null if we should assume we are not a descendant of the root. + // the root itself should be a node in this if non-null. + final Node pathFromRoot; + + ResolveSource(AbstractConfigObject root, Node pathFromRoot) { + this.root = root; + this.pathFromRoot = pathFromRoot; + } ResolveSource(AbstractConfigObject root) { this.root = root; - this.replacements = new IdentityHashMap(); + this.pathFromRoot = null; } - static private AbstractConfigValue findInObject(AbstractConfigObject obj, - ResolveContext context, SubstitutionExpression subst) + // if we replace the root with a non-object, use an empty + // object with nothing in it instead. + private AbstractConfigObject rootMustBeObj(Container value) { + if (value instanceof AbstractConfigObject) { + return (AbstractConfigObject) value; + } else { + return SimpleConfigObject.empty(); + } + } + + // as a side effect, findInObject() will have to resolve all parents of the + // child being peeked, but NOT the child itself. Caller has to resolve + // the child itself if needed. ValueWithPath.value can be null but + // the ValueWithPath instance itself should not be. + static private ValueWithPath findInObject(AbstractConfigObject obj, ResolveContext context, Path path) throws NotPossibleToResolve { - return obj.peekPath(subst.path(), context); + // resolve ONLY portions of the object which are along our path + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("*** finding '" + path + "' in " + obj); + AbstractConfigValue partiallyResolved = context.restrict(path).resolve(obj, new ResolveSource(obj)); + if (partiallyResolved instanceof AbstractConfigObject) { + return findInObject((AbstractConfigObject) partiallyResolved, path); + } else { + throw new ConfigException.BugOrBroken("resolved object to non-object " + obj + " to " + partiallyResolved); + } } - AbstractConfigValue lookupSubst(ResolveContext context, SubstitutionExpression subst, - int prefixLength) throws NotPossibleToResolve { + static private ValueWithPath findInObject(AbstractConfigObject obj, Path path) { + try { + // we'll fail if anything along the path can't + // be looked at without resolving. + return findInObject(obj, path, null); + } catch (ConfigException.NotResolved e) { + throw ConfigImpl.improveNotResolved(path, e); + } + } + + static private ValueWithPath findInObject(AbstractConfigObject obj, Path path, Node parents) { + String key = path.first(); + Path next = path.remainder(); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("*** looking up '" + key + "' in " + obj); + AbstractConfigValue v = obj.attemptPeekWithPartialResolve(key); + Node newParents = parents == null ? new Node(obj) : parents.prepend(obj); + + if (next == null) { + return new ValueWithPath(v, newParents); + } else { + if (v instanceof AbstractConfigObject) { + return findInObject((AbstractConfigObject) v, next, newParents); + } else { + return new ValueWithPath(null, newParents); + } + } + } + + ValueWithPath lookupSubst(ResolveContext context, SubstitutionExpression subst, + int prefixLength) + throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(context.depth(), "searching for " + subst); - context.trace(subst); - try { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), subst + " - looking up relative to file it occurred in"); - // First we look up the full path, which means relative to the - // included file if we were not a root file - AbstractConfigValue result = findInObject(root, context, subst); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), subst + " - looking up relative to file it occurred in"); + // First we look up the full path, which means relative to the + // included file if we were not a root file + ValueWithPath result = findInObject(root, context, subst.path()); - 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) + throw new ConfigException.BugOrBroken("findInObject() returned null"); - // replace the debug trace path - context.untrace(); - context.trace(unprefixed); + if (result.value == 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 (prefixLength > 0) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), unprefixed + " - looking up relative to parent file"); - result = findInObject(root, context, unprefixed); - } - - if (result == null && context.options().getUseSystemEnvironment()) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), unprefixed + " - looking up in system environment"); - result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context, - unprefixed); - } - } - - if (result != null) { + if (prefixLength > 0) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "recursively resolving " + result - + " which was the resolution of " + subst); - - result = context.resolve(result); + ConfigImpl.trace(context.depth(), unprefixed + " - looking up relative to parent file"); + result = findInObject(root, context, unprefixed); } - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "resolved to " + result); + if (result.value == null && context.options().getUseSystemEnvironment()) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), unprefixed + " - looking up in system environment"); + result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context, unprefixed); + } + } - return result; - } finally { - context.untrace(); + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(context.depth(), "resolved to " + result); + + return result; + } + + ResolveSource pushParent(Container parent) { + if (parent == null) + throw new ConfigException.BugOrBroken("can't push null parent"); + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("pushing parent " + parent + " ==root " + (parent == root) + " onto " + this); + + if (pathFromRoot == null) { + if (parent == root) { + return new ResolveSource(root, new Node(parent)); + } else { + if (ConfigImpl.traceSubstitutionsEnabled()) { + // this hasDescendant check is super-expensive so it's a + // trace message rather than an assertion + if (root.hasDescendant((AbstractConfigValue) parent)) + ConfigImpl.trace("***** BUG ***** tried to push parent " + parent + + " without having a path to it in " + this); + } + // ignore parents if we aren't proceeding from the + // root + return this; + } + } else { + Container parentParent = pathFromRoot.head(); + if (ConfigImpl.traceSubstitutionsEnabled()) { + // this hasDescendant check is super-expensive so it's a + // trace message rather than an assertion + if (parentParent != null && !parentParent.hasDescendant((AbstractConfigValue) parent)) + ConfigImpl.trace("***** BUG ***** trying to push non-child of " + parentParent + ", non-child was " + + parent); + } + + return new ResolveSource(root, pathFromRoot.prepend(parent)); } } - void replace(AbstractConfigValue value, ResolveReplacer replacer) { + ResolveSource popParent(Container parent) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace("Replacing " + value + "@" + System.identityHashCode(value) + " with " + replacer); - ResolveReplacer old = replacements.put(value, replacer); - if (old != null) - throw new ConfigException.BugOrBroken("should not have replaced the same value twice: " - + value); + ConfigImpl.trace("popping parent " + parent); + if (parent == null) + throw new ConfigException.BugOrBroken("can't pop null parent"); + else if (pathFromRoot == null) + return this; + else if (pathFromRoot.head() != parent) + throw new ConfigException.BugOrBroken("parent was not pushed, can't pop: " + parent); + else + return new ResolveSource(root, pathFromRoot.tail()); } - void unreplace(AbstractConfigValue value) { - ResolveReplacer replacer = replacements.remove(value); - if (replacer == null) - throw new ConfigException.BugOrBroken("unreplace() without replace(): " + value); - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace("Unreplacing " + value + "@" + System.identityHashCode(value) + " which was replaced by " - + replacer); + ResolveSource resetParents() { + if (pathFromRoot == null) + return this; + else + return new ResolveSource(root); } - private AbstractConfigValue replacement(ResolveContext context, AbstractConfigValue value) - throws NotPossibleToResolve { - ResolveReplacer replacer = replacements.get(value); - if (replacer == null) { + // returns null if the replacement results in deleting all the nodes. + private static Node replace(Node list, Container old, AbstractConfigValue replacement) { + Container child = list.head(); + if (child != old) + throw new ConfigException.BugOrBroken("Can only replace() the top node we're resolving; had " + child + + " on top and tried to replace " + old + " overall list was " + list); + Container parent = list.tail() == null ? null : list.tail().head(); + if (replacement == null || !(replacement instanceof Container)) { + if (parent == null) { + return null; + } else { + /* + * we are deleting the child from the stack of containers + * because it's either going away or not a container + */ + AbstractConfigValue newParent = parent.replaceChild((AbstractConfigValue) old, null); + + return replace(list.tail(), parent, newParent); + } + } else { + /* we replaced the container with another container */ + if (parent == null) { + return new Node((Container) replacement); + } else { + AbstractConfigValue newParent = parent.replaceChild((AbstractConfigValue) old, replacement); + Node newTail = replace(list.tail(), parent, newParent); + if (newTail != null) + return newTail.prepend((Container) replacement); + else + return new Node((Container) replacement); + } + } + } + + ResolveSource replaceCurrentParent(Container old, Container replacement) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("replaceCurrentParent old " + old + "@" + System.identityHashCode(old) + " replacement " + + replacement + "@" + System.identityHashCode(old) + " in " + this); + if (old == replacement) { + return this; + } else if (pathFromRoot != null) { + Node newPath = replace(pathFromRoot, old, (AbstractConfigValue) replacement); + if (ConfigImpl.traceSubstitutionsEnabled()) { + ConfigImpl.trace("replaced " + old + " with " + replacement + " in " + this); + ConfigImpl.trace("path was: " + pathFromRoot + " is now " + newPath); + } + // if we end up nuking the root object itself, we replace it with an + // empty root + if (newPath != null) + return new ResolveSource((AbstractConfigObject) newPath.last(), newPath); + else + return new ResolveSource(SimpleConfigObject.empty()); + } else { + if (old == root) { + return new ResolveSource(rootMustBeObj(replacement)); + } else { + throw new ConfigException.BugOrBroken("attempt to replace root " + root + " with " + replacement); + // return this; + } + } + } + + // replacement may be null to delete + ResolveSource replaceWithinCurrentParent(AbstractConfigValue old, AbstractConfigValue replacement) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace("replaceWithinCurrentParent old " + old + "@" + System.identityHashCode(old) + + " replacement " + replacement + "@" + System.identityHashCode(old) + " in " + this); + if (old == replacement) { + return this; + } else if (pathFromRoot != null) { + Container parent = pathFromRoot.head(); + AbstractConfigValue newParent = parent.replaceChild(old, replacement); + return replaceCurrentParent(parent, (newParent instanceof Container) ? (Container) newParent : null); + } else { + if (old == root && replacement instanceof Container) { + return new ResolveSource(rootMustBeObj((Container) replacement)); + } else { + throw new ConfigException.BugOrBroken("replace in parent not possible " + old + " with " + replacement + + " in " + this); + // return this; + } + } + } + + @Override + public String toString() { + return "ResolveSource(root=" + root + ", pathFromRoot=" + pathFromRoot + ")"; + } + + // a persistent list + static final class Node implements Iterable { + final T value; + final Node next; + + Node(T value, Node next) { + this.value = value; + this.next = next; + } + + Node(T value) { + this(value, null); + } + + Node prepend(T value) { + return new Node(value, this); + } + + T head() { return value; - } else { - AbstractConfigValue replacement = replacer.replace(context); - if (ConfigImpl.traceSubstitutionsEnabled() && value != replacement) { - ConfigImpl.trace(" when looking up substitutions " + context.traceString() + " replaced " + value - + " with " + replacement); + } + + Node tail() { + return next; + } + + T last() { + Node i = this; + while (i.next != null) + i = i.next; + return i.value; + } + + Node reverse() { + if (next == null) { + return this; + } else { + Node reversed = new Node(value); + Node i = next; + while (i != null) { + reversed = reversed.prepend(i.value); + i = i.next; + } + return reversed; } - return replacement; + } + + private static class NodeIterator implements Iterator { + Node current; + + NodeIterator(Node current) { + this.current = current; + } + + @Override + public T next() { + if (current == null) { + throw new NoSuchElementException(); + } else { + T result = current.value; + current = current.next; + return result; + } + } + + @Override + public boolean hasNext() { + return current != null; + } + + @Override + public void remove() { + throw new UnsupportedOperationException(); + } + } + + @Override + public Iterator iterator() { + return new NodeIterator(this); + } + + @Override + public String toString() { + StringBuffer sb = new StringBuffer(); + sb.append("["); + Node toAppendValue = this.reverse(); + while (toAppendValue != null) { + sb.append(toAppendValue.value.toString()); + if (toAppendValue.next != null) + sb.append(" <= "); + toAppendValue = toAppendValue.next; + } + sb.append("]"); + return sb.toString(); } } - /** - * Conceptually, this is key.value().resolveSubstitutions() but using the - * replacement for key.value() if any. - */ - AbstractConfigValue resolveCheckingReplacement(ResolveContext context, - AbstractConfigValue original) throws NotPossibleToResolve { - AbstractConfigValue replacement; + // value is allowed to be null + static final class ValueWithPath { + final AbstractConfigValue value; + final Node pathFromRoot; - replacement = replacement(context, original); + ValueWithPath(AbstractConfigValue value, Node pathFromRoot) { + this.value = value; + this.pathFromRoot = pathFromRoot; + } - if (replacement != original) { - // start over, checking if replacement was memoized - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "for resolution, replaced " + original + " with " + replacement); - return context.resolve(replacement); - } else { - AbstractConfigValue resolved; + ValueWithPath(AbstractConfigValue value) { + this(value, null); + } - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), - "resolving unreplaced " + original + " with trace '" + context.traceString() + "'"); - resolved = original.resolveSubstitutions(context); + ValueWithPath addParent(Container parent) { + if (pathFromRoot == null) + return new ValueWithPath(value, new Node(parent)); + else + return new ValueWithPath(value, pathFromRoot.prepend(parent)); + } - return resolved; + @Override + public String toString() { + return "ValueWithPath(value=" + value + ", pathFromRoot=" + pathFromRoot + ")"; } } } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index d197cd25..038e0b9e 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -18,7 +18,7 @@ import com.typesafe.config.ConfigRenderOptions; import com.typesafe.config.ConfigValue; import com.typesafe.config.ConfigValueType; -final class SimpleConfigList extends AbstractConfigValue implements ConfigList, Serializable { +final class SimpleConfigList extends AbstractConfigValue implements ConfigList, Container, Serializable { private static final long serialVersionUID = 2L; @@ -61,6 +61,23 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList, return ResolveStatus.fromBoolean(resolved); } + @Override + public SimpleConfigList replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) { + List newList = replaceChildInList(value, child, replacement); + if (newList == null) { + return null; + } else { + // we use the constructor flavor that will recompute the resolve + // status + return new SimpleConfigList(origin(), newList); + } + } + + @Override + public boolean hasDescendant(AbstractConfigValue descendant) { + return hasDescendantInList(value, descendant); + } + private SimpleConfigList modify(NoExceptionsModifier modifier, ResolveStatus newResolveStatus) { try { return modifyMayThrow(modifier, newResolveStatus); @@ -105,7 +122,8 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList, } @Override - SimpleConfigList resolveSubstitutions(final ResolveContext context) throws NotPossibleToResolve { + SimpleConfigList resolveSubstitutions(final ResolveContext context, ResolveSource source) + throws NotPossibleToResolve { if (resolved) return this; @@ -114,12 +132,13 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList, // so nothing to do. return this; } else { + final ResolveSource sourceWithParent = source.pushParent(this); try { return modifyMayThrow(new Modifier() { @Override public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) throws NotPossibleToResolve { - return context.resolve(v); + return context.resolve(v, sourceWithParent); } }, ResolveStatus.RESOLVED); diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java index 7667a20f..e652af61 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -198,6 +198,38 @@ final class SimpleConfigObject extends AbstractConfigObject implements Serializa return ResolveStatus.fromBoolean(resolved); } + @Override + public SimpleConfigObject replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) { + HashMap newChildren = new HashMap(value); + for (Map.Entry old : newChildren.entrySet()) { + if (old.getValue() == child) { + if (replacement != null) + old.setValue(replacement); + else + newChildren.remove(old.getKey()); + + return new SimpleConfigObject(origin(), newChildren, ResolveStatus.fromValues(newChildren.values()), + ignoresFallbacks); + } + } + throw new ConfigException.BugOrBroken("SimpleConfigObject.replaceChild did not find " + child + " in " + this); + } + + @Override + public boolean hasDescendant(AbstractConfigValue descendant) { + for (AbstractConfigValue child : value.values()) { + if (child == descendant) + return true; + } + // now do the expensive search + for (AbstractConfigValue child : value.values()) { + if (child instanceof Container && ((Container) child).hasDescendant(descendant)) + return true; + } + + return false; + } + @Override protected boolean ignoresFallbacks() { return ignoresFallbacks; @@ -313,21 +345,25 @@ final class SimpleConfigObject extends AbstractConfigObject implements Serializa } @Override - AbstractConfigObject resolveSubstitutions(final ResolveContext context) throws NotPossibleToResolve { + AbstractConfigObject resolveSubstitutions(final ResolveContext context, ResolveSource source) + throws NotPossibleToResolve { if (resolveStatus() == ResolveStatus.RESOLVED) return this; + final ResolveSource sourceWithParent = source.pushParent(this); + try { return modifyMayThrow(new Modifier() { @Override public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) throws NotPossibleToResolve { + if (context.isRestrictedToChild()) { if (key.equals(context.restrictToChild().first())) { Path remainder = context.restrictToChild().remainder(); if (remainder != null) { - return context.restrict(remainder).resolve(v); + return context.restrict(remainder).resolve(v, sourceWithParent); } else { // we don't want to resolve the leaf child. return v; @@ -338,7 +374,7 @@ final class SimpleConfigObject extends AbstractConfigObject implements Serializa } } else { // no restrictToChild, resolve everything - return context.unrestricted().resolve(v); + return context.unrestricted().resolve(v, sourceWithParent); } } diff --git a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala index f59b3251..be30a46e 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala @@ -360,13 +360,43 @@ class ConcatenationTest extends TestUtils { // to get there, see https://github.com/typesafehub/config/issues/160 @Test def plusEqualsMultipleTimesNestedInPlusEquals() { - System.err.println("==============") val e = intercept[ConfigException.Parse] { val conf = parseConfig("""x += { a += 1, a += 2, a += 3 } """).resolve() assertEquals(Seq(1, 2, 3), conf.getObjectList("x").asScala.toVector(0).toConfig.getIntList("a").asScala.toList) } assertTrue(e.getMessage.contains("limitation")) - System.err.println("==============") + } + + // from https://github.com/typesafehub/config/issues/177 + @Test + def arrayConcatenationInDoubleNestedDelayedMerge() { + val unresolved = parseConfig("""d { x = [] }, c : ${d}, c { x += 1, x += 2 }""") + val conf = unresolved.resolve() + assertEquals(Seq(1, 2), conf.getIntList("c.x").asScala) + } + + // from https://github.com/typesafehub/config/issues/177 + @Test + def arrayConcatenationAsPartOfDelayedMerge() { + val unresolved = parseConfig(""" c { x: [], x : ${c.x}[1], x : ${c.x}[2] }""") + val conf = unresolved.resolve() + assertEquals(Seq(1, 2), conf.getIntList("c.x").asScala) + } + + // from https://github.com/typesafehub/config/issues/177 + @Test + def arrayConcatenationInDoubleNestedDelayedMerge2() { + val unresolved = parseConfig("""d { x = [] }, c : ${d}, c { x : ${c.x}[1], x : ${c.x}[2] }""") + val conf = unresolved.resolve() + assertEquals(Seq(1, 2), conf.getIntList("c.x").asScala) + } + + // from https://github.com/typesafehub/config/issues/177 + @Test + def arrayConcatenationInTripleNestedDelayedMerge() { + val unresolved = parseConfig("""{ r: { d.x=[] }, q: ${r}, q : { d { x = [] }, c : ${q.d}, c { x : ${q.c.x}[1], x : ${q.c.x}[2] } } }""") + val conf = unresolved.resolve() + assertEquals(Seq(1, 2), conf.getIntList("q.c.x").asScala) } @Test 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 2c3b067c..2eff0545 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -207,6 +207,16 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(2, resolved.getInt("b")) } + @Test + def throwOnIncrediblyTrivialCycle() { + val s = subst("a") + val e = intercept[ConfigException.UnresolvedSubstitution] { + val v = resolveWithoutFallbacks(s, parseObject("a: ${a}")) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("cycle")) + assertTrue("Wrong exception: " + e.getMessage, e.getMessage().contains("${a}")) + } + private val substCycleObject = { parseObject(""" { @@ -476,9 +486,9 @@ class ConfigSubstitutionTest extends TestUtils { val resolved = resolveWithoutFallbacks(delayedMergeObjectResolveProblem5) - assertEquals(2, resolved.getInt("item1.b")) - assertEquals(2, resolved.getInt("item2.b")) - assertEquals(2, resolved.getInt("defaults.a")) + assertEquals("item1.b", 2, resolved.getInt("item1.b")) + assertEquals("item2.b", 2, resolved.getInt("item2.b")) + assertEquals("defaults.a", 2, resolved.getInt("defaults.a")) } private val delayedMergeObjectResolveProblem6 = { From c8f42ef92dd419f99c8eaebdc94f24c8feb1c4db Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Mon, 13 Oct 2014 11:23:07 -0400 Subject: [PATCH 07/14] Make ResolveContext immutable This is a mechanical refactoring not intended to change behavior. Not very efficiently implemented but that's fine for now. --- .../config/impl/AbstractConfigObject.java | 3 +- .../config/impl/AbstractConfigValue.java | 4 +- .../config/impl/ConfigConcatenation.java | 19 +++-- .../config/impl/ConfigDelayedMerge.java | 29 ++++--- .../config/impl/ConfigDelayedMergeObject.java | 13 ++- .../typesafe/config/impl/ConfigReference.java | 74 ++++++++--------- .../typesafe/config/impl/ResolveContext.java | 79 +++++++++++-------- .../typesafe/config/impl/ResolveMemos.java | 14 +++- .../typesafe/config/impl/ResolveResult.java | 43 ++++++++++ .../typesafe/config/impl/ResolveSource.java | 58 ++++++++++---- .../config/impl/SimpleConfigList.java | 35 +++++--- .../config/impl/SimpleConfigObject.java | 72 ++++++++++------- 12 files changed, 288 insertions(+), 155 deletions(-) create mode 100644 config/src/main/java/com/typesafe/config/impl/ResolveResult.java diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index bcda80d7..a6863672 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -177,7 +177,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements Confi } @Override - abstract AbstractConfigObject resolveSubstitutions(ResolveContext context, ResolveSource source) + abstract ResolveResult resolveSubstitutions(ResolveContext context, + ResolveSource source) throws NotPossibleToResolve; @Override diff --git a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index 1eb05c81..0720417e 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -72,9 +72,9 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue { * where to look up values * @return a new value if there were changes, or this if no changes */ - AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { - return this; + return ResolveResult.make(context, this); } ResolveStatus resolveStatus() { diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java index e4f031eb..88f34ca9 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java @@ -170,7 +170,8 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab } @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) + throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) { int indent = context.depth() + 2; ConfigImpl.trace(indent - 1, "concatenation has " + pieces.size() + " pieces:"); @@ -185,12 +186,17 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab // content of ConfigConcatenation should not need to replaceChild, // but if it did we'd have to do this. ResolveSource sourceWithParent = source; // .pushParent(this); + ResolveContext newContext = context; List resolved = new ArrayList(pieces.size()); for (AbstractConfigValue p : pieces) { // to concat into a string we have to do a full resolve, - // so unrestrict the context - AbstractConfigValue r = context.unrestricted().resolve(p, sourceWithParent); + // so unrestrict the context, then put restriction back afterward + Path restriction = newContext.restrictToChild(); + ResolveResult result = newContext.unrestricted() + .resolve(p, sourceWithParent); + AbstractConfigValue r = result.value; + newContext = result.context.restrict(restriction); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(context.depth(), "resolved concat piece to " + r); if (r == null) { @@ -205,11 +211,12 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab // if unresolved is allowed we can just become another // ConfigConcatenation if (joined.size() > 1 && context.options().getAllowUnresolved()) - return new ConfigConcatenation(this.origin(), joined); + return ResolveResult.make(newContext, new ConfigConcatenation(this.origin(), joined)); else if (joined.isEmpty()) - return null; // we had just a list of optional references using ${?} + // we had just a list of optional references using ${?} + return ResolveResult.make(newContext, null); else if (joined.size() == 1) - return joined.get(0); + return ResolveResult.make(newContext, joined.get(0)); else throw new ConfigException.BugOrBroken("Bug in the library; resolved list was joined to too many values: " + joined); 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 32a8ea65..2893faf8 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -54,13 +54,14 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl } @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { return resolveSubstitutions(this, stack, context, source); } // static method also used by ConfigDelayedMergeObject - static AbstractConfigValue resolveSubstitutions(ReplaceableMergeStack replaceable, List stack, + static ResolveResult resolveSubstitutions(ReplaceableMergeStack replaceable, + List stack, ResolveContext context, ResolveSource source) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) { ConfigImpl.trace(context.depth(), "delayed merge stack has " + stack.size() + " items:"); @@ -77,6 +78,7 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // is non-null, or resolve options allow partial resolves, // we may remain a delayed merge though. + ResolveContext newContext = context; int count = 0; AbstractConfigValue merged = null; for (AbstractConfigValue end : stack) { @@ -92,7 +94,7 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl AbstractConfigValue remainder = replaceable.makeReplacement(context, count + 1); if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "remainder portion: " + remainder); + ConfigImpl.trace(newContext.depth(), "remainder portion: " + remainder); // If, while resolving 'end' we come back to the same // merge stack, we only want to look _below_ 'end' @@ -101,40 +103,43 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl // the remainder of the stack below this one. if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "building sourceForEnd"); + ConfigImpl.trace(newContext.depth(), "building sourceForEnd"); // we resetParents() here because we'll be resolving "end" // against a root which does NOT contain "end" sourceForEnd = source.replaceWithinCurrentParent((AbstractConfigValue) replaceable, remainder); if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), " sourceForEnd before reset parents but after replace: " + ConfigImpl.trace(newContext.depth(), " sourceForEnd before reset parents but after replace: " + sourceForEnd); sourceForEnd = sourceForEnd.resetParents(); } else { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl - .trace(context.depth(), "will resolve end against the original source with parent pushed"); +.trace(newContext.depth(), + "will resolve end against the original source with parent pushed"); sourceForEnd = source.pushParent(replaceable); } if (ConfigImpl.traceSubstitutionsEnabled()) { - ConfigImpl.trace(context.depth(), "sourceForEnd =" + sourceForEnd); + ConfigImpl.trace(newContext.depth(), "sourceForEnd =" + sourceForEnd); } if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "Resolving highest-priority item in delayed merge " + end + ConfigImpl.trace(newContext.depth(), "Resolving highest-priority item in delayed merge " + end + " against " + sourceForEnd + " endWasRemoved=" + (source != sourceForEnd)); - AbstractConfigValue resolvedEnd = context.resolve(end, sourceForEnd); + ResolveResult result = newContext.resolve(end, sourceForEnd); + AbstractConfigValue resolvedEnd = result.value; + newContext = result.context; if (resolvedEnd != null) { if (merged == null) { merged = resolvedEnd; } else { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth() + 1, "merging " + merged + " with fallback " + resolvedEnd); + ConfigImpl.trace(newContext.depth() + 1, "merging " + merged + " with fallback " + resolvedEnd); merged = merged.withFallback(resolvedEnd); } } @@ -142,10 +147,10 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl count += 1; if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "stack merged, yielding: " + merged); + ConfigImpl.trace(newContext.depth(), "stack merged, yielding: " + merged); } - return merged; + return ResolveResult.make(newContext, merged); } @Override 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 f9afd6d7..7aa3ef97 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -50,14 +50,11 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements Unm } @Override - AbstractConfigObject resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { - AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(this, stack, context, source); - if (merged instanceof AbstractConfigObject) { - return (AbstractConfigObject) merged; - } else { - throw new ConfigException.BugOrBroken( - "somehow brokenly merged an object and didn't get an object, got " + merged); - } + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) + throws NotPossibleToResolve { + ResolveResult merged = ConfigDelayedMerge.resolveSubstitutions(this, stack, + context, source); + return merged.asObjectResult(); } @Override diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java index 963157cb..8d3c7c0c 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigReference.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigReference.java @@ -65,49 +65,49 @@ final class ConfigReference extends AbstractConfigValue implements Unmergeable { // This way it's impossible for NotPossibleToResolve to "escape" since // any failure to resolve has to start with a ConfigReference. @Override - AbstractConfigValue resolveSubstitutions(ResolveContext context, ResolveSource source) { - context.addCycleMarker(this); + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) { + ResolveContext newContext = context.addCycleMarker(this); + AbstractConfigValue v; try { - AbstractConfigValue v; - try { - ResolveSource.ValueWithPath valueWithPath = source.lookupSubst(context, expr, prefixLength); + ResolveSource.ResultWithPath resultWithPath = source.lookupSubst(newContext, expr, prefixLength); + newContext = resultWithPath.result.context; - if (valueWithPath.value != null) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "recursively resolving " + valueWithPath - + " which was the resolution of " + expr + " against " + source); - - ResolveSource recursiveResolveSource = (new ResolveSource( - (AbstractConfigObject) valueWithPath.pathFromRoot.last(), valueWithPath.pathFromRoot)); - - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "will recursively resolve against " + recursiveResolveSource); - - v = context.resolve(valueWithPath.value, recursiveResolveSource); - } else { - v = null; - } - } catch (NotPossibleToResolve e) { + if (resultWithPath.result.value != null) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), - "not possible to resolve " + expr + ", cycle involved: " + e.traceString()); - if (expr.optional()) - v = null; - else - throw new ConfigException.UnresolvedSubstitution(origin(), expr - + " was part of a cycle of substitutions involving " + e.traceString(), e); - } + ConfigImpl.trace(newContext.depth(), "recursively resolving " + resultWithPath + + " which was the resolution of " + expr + " against " + source); - if (v == null && !expr.optional()) { - if (context.options().getAllowUnresolved()) - return this; - else - throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString()); + ResolveSource recursiveResolveSource = (new ResolveSource( + (AbstractConfigObject) resultWithPath.pathFromRoot.last(), resultWithPath.pathFromRoot)); + + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(newContext.depth(), "will recursively resolve against " + recursiveResolveSource); + + ResolveResult result = newContext.resolve(resultWithPath.result.value, + recursiveResolveSource); + v = result.value; + newContext = result.context; } else { - return v; + v = null; } - } finally { - context.removeCycleMarker(this); + } catch (NotPossibleToResolve e) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(newContext.depth(), + "not possible to resolve " + expr + ", cycle involved: " + e.traceString()); + if (expr.optional()) + v = null; + else + throw new ConfigException.UnresolvedSubstitution(origin(), expr + + " was part of a cycle of substitutions involving " + e.traceString(), e); + } + + if (v == null && !expr.optional()) { + if (newContext.options().getAllowUnresolved()) + return ResolveResult.make(newContext.removeCycleMarker(this), this); + else + throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString()); + } else { + return ResolveResult.make(newContext.removeCycleMarker(this), v); } } 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 49b1ec22..076d07fe 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -11,8 +11,6 @@ import com.typesafe.config.ConfigResolveOptions; import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; final class ResolveContext { - // this is unfortunately mutable so should only be shared among - // ResolveContext in the same traversal. final private ResolveMemos memos; final private ConfigResolveOptions options; @@ -34,31 +32,46 @@ final class ResolveContext { this.memos = memos; this.options = options; this.restrictToChild = restrictToChild; - this.resolveStack = resolveStack; - this.cycleMarkers = cycleMarkers; + this.resolveStack = Collections.unmodifiableList(resolveStack); + this.cycleMarkers = Collections.unmodifiableSet(cycleMarkers); + } + + private static Set newCycleMarkers() { + return Collections.newSetFromMap(new IdentityHashMap()); } ResolveContext(ConfigResolveOptions options, Path restrictToChild) { // LinkedHashSet keeps the traversal order which is at least useful // in error messages if nothing else - this(new ResolveMemos(), options, restrictToChild, new ArrayList(), Collections - .newSetFromMap(new IdentityHashMap())); + this(new ResolveMemos(), options, restrictToChild, new ArrayList(), newCycleMarkers()); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "ResolveContext restrict to child " + restrictToChild); } - void addCycleMarker(AbstractConfigValue value) { + ResolveContext addCycleMarker(AbstractConfigValue value) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "++ Cycle marker " + value + "@" + System.identityHashCode(value)); if (cycleMarkers.contains(value)) throw new ConfigException.BugOrBroken("Added cycle marker twice " + value); - cycleMarkers.add(value); + Set copy = newCycleMarkers(); + copy.addAll(cycleMarkers); + copy.add(value); + return new ResolveContext(memos, options, restrictToChild, resolveStack, copy); } - void removeCycleMarker(AbstractConfigValue value) { - cycleMarkers.remove(value); + ResolveContext removeCycleMarker(AbstractConfigValue value) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "-- Cycle marker " + value + "@" + System.identityHashCode(value)); + + Set copy = newCycleMarkers(); + copy.addAll(cycleMarkers); + copy.remove(value); + return new ResolveContext(memos, options, restrictToChild, resolveStack, copy); + } + + private ResolveContext memoize(MemoKey key, AbstractConfigValue value) { + ResolveMemos changed = memos.put(key, value); + return new ResolveContext(changed, options, restrictToChild, resolveStack, cycleMarkers); } ConfigResolveOptions options() { @@ -73,6 +86,7 @@ final class ResolveContext { return restrictToChild; } + // restrictTo may be null to unrestrict ResolveContext restrict(Path restrictTo) { if (restrictTo == restrictToChild) return this; @@ -98,16 +112,20 @@ final class ResolveContext { return sb.toString(); } - private void pushTrace(AbstractConfigValue value) { + private ResolveContext pushTrace(AbstractConfigValue value) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "pushing trace " + value); - resolveStack.add(value); + List copy = new ArrayList(resolveStack); + copy.add(value); + return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers); } - private void popTrace() { - AbstractConfigValue old = resolveStack.remove(resolveStack.size() - 1); + ResolveContext popTrace() { + List copy = new ArrayList(resolveStack); + AbstractConfigValue old = copy.remove(resolveStack.size() - 1); if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(depth(), "popped trace " + old); + ConfigImpl.trace(depth() - 1, "popped trace " + old); + return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers); } int depth() { @@ -116,21 +134,15 @@ final class ResolveContext { return resolveStack.size(); } - AbstractConfigValue resolve(AbstractConfigValue original, ResolveSource source) throws NotPossibleToResolve { + ResolveResult resolve(AbstractConfigValue original, ResolveSource source) + throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl .trace(depth(), "resolving " + original + " restrictToChild=" + restrictToChild + " in " + source); - AbstractConfigValue resolved; - pushTrace(original); - try { - resolved = realResolve(original, source); - } finally { - popTrace(); - } - return resolved; + return pushTrace(original).realResolve(original, source).popTrace(); } - private AbstractConfigValue realResolve(AbstractConfigValue original, ResolveSource source) + private ResolveResult realResolve(AbstractConfigValue original, ResolveSource source) throws NotPossibleToResolve { // a fully-resolved (no restrictToChild) object can satisfy a // request for a restricted object, so always check that first. @@ -151,7 +163,7 @@ final class ResolveContext { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "using cached resolution " + cached + " for " + original + " restrictToChild " + restrictToChild()); - return cached; + return ResolveResult.make(this, cached); } else { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), @@ -164,12 +176,15 @@ final class ResolveContext { throw new NotPossibleToResolve(this); } - AbstractConfigValue resolved = original.resolveSubstitutions(this, source); + ResolveResult result = original.resolveSubstitutions(this, source); + AbstractConfigValue resolved = result.value; if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "resolved to " + resolved + "@" + System.identityHashCode(resolved) + " from " + original + "@" + System.identityHashCode(resolved)); + ResolveContext withMemo = result.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 @@ -178,7 +193,7 @@ final class ResolveContext { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); - memos.put(fullKey, resolved); + withMemo = withMemo.memoize(fullKey, resolved); } else { // if we have an unresolved object then either we did a // partial resolve restricted to a certain child, or we are @@ -191,19 +206,19 @@ final class ResolveContext { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + restrictedKey + " result " + resolved); - memos.put(restrictedKey, resolved); + withMemo = withMemo.memoize(restrictedKey, resolved); } else if (options().getAllowUnresolved()) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); - memos.put(fullKey, resolved); + withMemo = withMemo.memoize(fullKey, resolved); } else { throw new ConfigException.BugOrBroken( "resolveSubstitutions() did not give us a resolved object"); } } - return resolved; + return ResolveResult.make(withMemo, resolved); } } @@ -213,7 +228,7 @@ final class ResolveContext { ResolveContext context = new ResolveContext(options, null /* restrictToChild */); try { - return context.resolve(value, source); + return context.resolve(value, source).value; } catch (NotPossibleToResolve e) { // ConfigReference was supposed to catch NotPossibleToResolve throw new ConfigException.BugOrBroken( diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveMemos.java b/config/src/main/java/com/typesafe/config/impl/ResolveMemos.java index dc43d727..cf98a900 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveMemos.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveMemos.java @@ -13,15 +13,23 @@ final class ResolveMemos { // rather than ConfigNull) so this map can have null values. final private Map memos; + private ResolveMemos(Map memos) { + this.memos = memos; + } + ResolveMemos() { - this.memos = new HashMap(); + this(new HashMap()); } AbstractConfigValue get(MemoKey key) { return memos.get(key); } - void put(MemoKey key, AbstractConfigValue value) { - memos.put(key, value); + ResolveMemos put(MemoKey key, AbstractConfigValue value) { + // completely inefficient, but so far nobody cares about resolve() + // performance, we can clean it up someday... + Map copy = new HashMap(memos); + copy.put(key, value); + return new ResolveMemos(copy); } } diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveResult.java b/config/src/main/java/com/typesafe/config/impl/ResolveResult.java new file mode 100644 index 00000000..28e26c00 --- /dev/null +++ b/config/src/main/java/com/typesafe/config/impl/ResolveResult.java @@ -0,0 +1,43 @@ +package com.typesafe.config.impl; + +import com.typesafe.config.ConfigException; + +// value is allowed to be null +final class ResolveResult { + public final ResolveContext context; + public final V value; + + private ResolveResult(ResolveContext context, V value) { + this.context = context; + this.value = value; + } + + static ResolveResult make(ResolveContext context, V value) { + return new ResolveResult(context, value); + } + + // better option? we don't have variance + @SuppressWarnings("unchecked") + ResolveResult asObjectResult() { + if (!(value instanceof AbstractConfigObject)) + throw new ConfigException.BugOrBroken("Expecting a resolve result to be an object, but it was " + value); + Object o = this; + return (ResolveResult) o; + } + + // better option? we don't have variance + @SuppressWarnings("unchecked") + ResolveResult asValueResult() { + Object o = this; + return (ResolveResult) o; + } + + ResolveResult popTrace() { + return make(context.popTrace(), value); + } + + @Override + public String toString() { + return "ResolveResult(" + value + ")"; + } +} diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index 514c38a7..6eea30a3 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -41,14 +41,18 @@ final class ResolveSource { // child being peeked, but NOT the child itself. Caller has to resolve // the child itself if needed. ValueWithPath.value can be null but // the ValueWithPath instance itself should not be. - static private ValueWithPath findInObject(AbstractConfigObject obj, ResolveContext context, Path path) + static private ResultWithPath findInObject(AbstractConfigObject obj, ResolveContext context, Path path) throws NotPossibleToResolve { // resolve ONLY portions of the object which are along our path if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace("*** finding '" + path + "' in " + obj); - AbstractConfigValue partiallyResolved = context.restrict(path).resolve(obj, new ResolveSource(obj)); - if (partiallyResolved instanceof AbstractConfigObject) { - return findInObject((AbstractConfigObject) partiallyResolved, path); + Path restriction = context.restrictToChild(); + ResolveResult partiallyResolved = context.restrict(path).resolve(obj, + new ResolveSource(obj)); + ResolveContext newContext = partiallyResolved.context.restrict(restriction); + if (partiallyResolved.value instanceof AbstractConfigObject) { + ValueWithPath pair = findInObject((AbstractConfigObject) partiallyResolved.value, path); + return new ResultWithPath(ResolveResult.make(newContext, pair.value), pair.pathFromRoot); } else { throw new ConfigException.BugOrBroken("resolved object to non-object " + obj + " to " + partiallyResolved); } @@ -83,7 +87,7 @@ final class ResolveSource { } } - ValueWithPath lookupSubst(ResolveContext context, SubstitutionExpression subst, + ResultWithPath lookupSubst(ResolveContext context, SubstitutionExpression subst, int prefixLength) throws NotPossibleToResolve { if (ConfigImpl.traceSubstitutionsEnabled()) @@ -93,12 +97,9 @@ final class ResolveSource { ConfigImpl.trace(context.depth(), subst + " - looking up relative to file it occurred in"); // First we look up the full path, which means relative to the // included file if we were not a root file - ValueWithPath result = findInObject(root, context, subst.path()); + ResultWithPath result = findInObject(root, context, subst.path()); - if (result == null) - throw new ConfigException.BugOrBroken("findInObject() returned null"); - - if (result.value == null) { + if (result.result.value == 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. @@ -106,19 +107,20 @@ final class ResolveSource { if (prefixLength > 0) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), unprefixed + " - looking up relative to parent file"); - result = findInObject(root, context, unprefixed); + ConfigImpl.trace(result.result.context.depth(), unprefixed + + " - looking up relative to parent file"); + result = findInObject(root, result.result.context, unprefixed); } - if (result.value == null && context.options().getUseSystemEnvironment()) { + if (result.result.value == null && result.result.context.options().getUseSystemEnvironment()) { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), unprefixed + " - looking up in system environment"); + ConfigImpl.trace(result.result.context.depth(), unprefixed + " - looking up in system environment"); result = findInObject(ConfigImpl.envVariablesAsConfigObject(), context, unprefixed); } } if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(context.depth(), "resolved to " + result); + ConfigImpl.trace(result.result.context.depth(), "resolved to " + result); return result; } @@ -391,4 +393,30 @@ final class ResolveSource { return "ValueWithPath(value=" + value + ", pathFromRoot=" + pathFromRoot + ")"; } } + + static final class ResultWithPath { + final ResolveResult result; + final Node pathFromRoot; + + ResultWithPath(ResolveResult result, Node pathFromRoot) { + this.result = result; + this.pathFromRoot = pathFromRoot; + } + + ResultWithPath(ResolveResult result) { + this(result, null); + } + + ResultWithPath addParent(Container parent) { + if (pathFromRoot == null) + return new ResultWithPath(result, new Node(parent)); + else + return new ResultWithPath(result, pathFromRoot.prepend(parent)); + } + + @Override + public String toString() { + return "ResultWithPath(result=" + result + ", pathFromRoot=" + pathFromRoot + ")"; + } + } } diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index 038e0b9e..77ad5c6a 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -121,27 +121,38 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList, } } + private static class ResolveModifier implements Modifier { + ResolveContext context; + final ResolveSource source; + ResolveModifier(ResolveContext context, ResolveSource source) { + this.context = context; + this.source = source; + } + + @Override + public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) + throws NotPossibleToResolve { + ResolveResult result = context.resolve(v, source); + context = result.context; + return result.value; + } + } + @Override - SimpleConfigList resolveSubstitutions(final ResolveContext context, ResolveSource source) + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { if (resolved) - return this; + return ResolveResult.make(context, this); if (context.isRestrictedToChild()) { // if a list restricts to a child path, then it has no child paths, // so nothing to do. - return this; + return ResolveResult.make(context, this); } else { - final ResolveSource sourceWithParent = source.pushParent(this); try { - return modifyMayThrow(new Modifier() { - @Override - public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) - throws NotPossibleToResolve { - return context.resolve(v, sourceWithParent); - } - - }, ResolveStatus.RESOLVED); + ResolveModifier modifier = new ResolveModifier(context, source.pushParent(this)); + SimpleConfigList value = modifyMayThrow(modifier, ResolveStatus.RESOLVED); + return ResolveResult.make(modifier.context, value); } catch (NotPossibleToResolve e) { throw e; } catch (RuntimeException e) { diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java index e652af61..fbab0a46 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -344,41 +344,59 @@ final class SimpleConfigObject extends AbstractConfigObject implements Serializa } } + private static final class ResolveModifier implements Modifier { + + final Path originalRestrict; + ResolveContext context; + final ResolveSource source; + + ResolveModifier(ResolveContext context, ResolveSource source) { + this.context = context; + this.source = source; + originalRestrict = context.restrictToChild(); + } + + @Override + public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) throws NotPossibleToResolve { + if (context.isRestrictedToChild()) { + if (key.equals(context.restrictToChild().first())) { + Path remainder = context.restrictToChild().remainder(); + if (remainder != null) { + ResolveResult result = context.restrict(remainder).resolve(v, + source); + context = result.context.unrestricted().restrict(originalRestrict); + return result.value; + } else { + // we don't want to resolve the leaf child. + return v; + } + } else { + // not in the restrictToChild path + return v; + } + } else { + // no restrictToChild, resolve everything + ResolveResult result = context.unrestricted().resolve(v, source); + context = result.context.unrestricted().restrict(originalRestrict); + return result.value; + } + } + + } + @Override - AbstractConfigObject resolveSubstitutions(final ResolveContext context, ResolveSource source) + ResolveResult resolveSubstitutions(ResolveContext context, ResolveSource source) throws NotPossibleToResolve { if (resolveStatus() == ResolveStatus.RESOLVED) - return this; + return ResolveResult.make(context, this); final ResolveSource sourceWithParent = source.pushParent(this); try { - return modifyMayThrow(new Modifier() { + ResolveModifier modifier = new ResolveModifier(context, sourceWithParent); - @Override - public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v) - throws NotPossibleToResolve { - - if (context.isRestrictedToChild()) { - if (key.equals(context.restrictToChild().first())) { - Path remainder = context.restrictToChild().remainder(); - if (remainder != null) { - return context.restrict(remainder).resolve(v, sourceWithParent); - } else { - // we don't want to resolve the leaf child. - return v; - } - } else { - // not in the restrictToChild path - return v; - } - } else { - // no restrictToChild, resolve everything - return context.unrestricted().resolve(v, sourceWithParent); - } - } - - }); + AbstractConfigValue value = modifyMayThrow(modifier); + return ResolveResult.make(modifier.context, value).asObjectResult(); } catch (NotPossibleToResolve e) { throw e; } catch (RuntimeException e) { From 0a828f500bb265ca2545c4fc15f58e0aa06c8500 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Tue, 14 Oct 2014 03:46:38 -0400 Subject: [PATCH 08/14] Implement equals and hashCode for ResolveSource Useful for debugging or whatever. --- .../com/typesafe/config/impl/ResolveSource.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index 6eea30a3..dd6a4d74 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -275,6 +275,23 @@ final class ResolveSource { final T value; final Node next; + @Override + public int hashCode() { + return value.hashCode() + 41 * (41 + next.hashCode()); + } + + @Override + public boolean equals(Object other) { + if (other instanceof Node) { + if (value == ((Node) other).value) + return true; + else + return value.equals(((Node) other).value); + } else { + return false; + } + } + Node(T value, Node next) { this.value = value; this.next = next; From da494cd8a01d49a1f08107201fa4e1fc36d5aa1e Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Wed, 22 Oct 2014 13:54:08 -0400 Subject: [PATCH 09/14] Spec: must memoize when resolving substitutions --- HOCON.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/HOCON.md b/HOCON.md index d439fed6..6a00fefc 100644 --- a/HOCON.md +++ b/HOCON.md @@ -825,6 +825,15 @@ 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). +Implementations must set both `a` and `b` to the same value in +this case, however. In practice this means that all substitutions +must be memoized (resolved once, with the result +retained). Memoization should be keyed by the substitution +"instance" (the specific occurrence of the `${}` expression) +rather than by the path inside the `${}` expression, because +substitutions may be resolved differently depending on their +position in the file. + ### Includes #### Include syntax From 1a65f861b272284f870fcc12bf46c3b144aaba97 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Wed, 22 Oct 2014 13:54:48 -0400 Subject: [PATCH 10/14] ConfigDelayedMerge.java: Fix indentation --- .../main/java/com/typesafe/config/impl/ConfigDelayedMerge.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 2893faf8..88cce187 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -116,8 +116,7 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements Unmergeabl sourceForEnd = sourceForEnd.resetParents(); } else { if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl -.trace(newContext.depth(), + ConfigImpl.trace(newContext.depth(), "will resolve end against the original source with parent pushed"); sourceForEnd = source.pushParent(replaceable); From c3325a8966a71550c9fe319b2d366ff7f89fff0f Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Wed, 22 Oct 2014 14:24:33 -0400 Subject: [PATCH 11/14] Remove "root" from MemoKey This was just wrong in the new setup; we change the root constantly. The MemoKey should be per-conceptual-resolve which is per-ResolveContext, not per-root. This commit fixes the tests in which we failed to memoize and thus got different results for the same ConfigReference at different times in the resolution process. But it breaks ConfigSubstitutionTest.avoidDelayedMergeObjectResolveProblem5. --- config/src/main/java/com/typesafe/config/impl/MemoKey.java | 7 +------ .../main/java/com/typesafe/config/impl/ResolveContext.java | 4 ++-- 2 files changed, 3 insertions(+), 8 deletions(-) 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 d948aed0..cd843dad 100644 --- a/config/src/main/java/com/typesafe/config/impl/MemoKey.java +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -2,20 +2,17 @@ package com.typesafe.config.impl; /** The key used to memoize already-traversed nodes when resolving substitutions */ final class MemoKey { - MemoKey(AbstractConfigValue root, AbstractConfigValue value, Path restrictToChildOrNull) { - this.root = root; + MemoKey(AbstractConfigValue value, Path restrictToChildOrNull) { this.value = value; this.restrictToChildOrNull = restrictToChildOrNull; } - final private AbstractConfigValue root; final private AbstractConfigValue value; final private Path restrictToChildOrNull; @Override public final int hashCode() { int h = System.identityHashCode(value); - h = h + 41 * (41 + root.hashCode()); if (restrictToChildOrNull != null) { return h + 41 * (41 + restrictToChildOrNull.hashCode()); } else { @@ -29,8 +26,6 @@ final class MemoKey { MemoKey o = (MemoKey) other; if (o.value != this.value) return false; - else if (o.root != this.root) - return false; else if (o.restrictToChildOrNull == this.restrictToChildOrNull) return true; else if (o.restrictToChildOrNull == null || this.restrictToChildOrNull == null) 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 076d07fe..cf669912 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -146,7 +146,7 @@ final class ResolveContext { throws NotPossibleToResolve { // a fully-resolved (no restrictToChild) object can satisfy a // request for a restricted object, so always check that first. - final MemoKey fullKey = new MemoKey(source.root, original, null); + final MemoKey fullKey = new MemoKey(original, null); MemoKey restrictedKey = null; AbstractConfigValue cached = memos.get(fullKey); @@ -155,7 +155,7 @@ final class ResolveContext { // compute the restrictToChild object so use a more limited // memo key if (cached == null && isRestrictedToChild()) { - restrictedKey = new MemoKey(source.root, original, restrictToChild()); + restrictedKey = new MemoKey(original, restrictToChild()); cached = memos.get(restrictedKey); } From 53d642d93f9a47b8c453f90e3da1e85e95930933 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sat, 27 Dec 2014 16:06:40 -0500 Subject: [PATCH 12/14] Fix warning about use of octal escape in string literal --- .../src/test/scala/com/typesafe/config/impl/TokenizerTest.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/src/test/scala/com/typesafe/config/impl/TokenizerTest.scala b/config/src/test/scala/com/typesafe/config/impl/TokenizerTest.scala index f7da4233..882f0cb4 100644 --- a/config/src/test/scala/com/typesafe/config/impl/TokenizerTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/TokenizerTest.scala @@ -145,7 +145,7 @@ class TokenizerTest extends TestUtils { assertEquals('6', "\\u0046"(5)) val tests = List[UnescapeTest]((""" "" """, ""), - (" \"\\u0000\" ", "\0"), // nul byte + (" \"\\u0000\" ", Character.toString(0)), // nul byte (""" "\"\\\/\b\f\n\r\t" """, "\"\\/\b\f\n\r\t"), ("\"\\u0046\"", "F"), ("\"\\u0046\\u0046\"", "FF")) From 485f910d55be97cd9a3b502e65544741758974e4 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sat, 27 Dec 2014 16:26:52 -0500 Subject: [PATCH 13/14] Change expected result of delayedMergeObjectResolveProblem5 Our "do not look at what we're resolving in order to resolve it" algorithm is now more robust so we get a different answer. --- .../com/typesafe/config/impl/ConfigSubstitutionTest.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 2eff0545..e0dcdcc5 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -466,7 +466,8 @@ class ConfigSubstitutionTest extends TestUtils { private val delayedMergeObjectResolveProblem5 = { parseObject(""" defaults { - a = ${item1.b} // tricky cycle + a = ${item1.b} // tricky cycle - we won't see ${defaults} + // as we resolve this b = 2 } @@ -488,7 +489,7 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals("item1.b", 2, resolved.getInt("item1.b")) assertEquals("item2.b", 2, resolved.getInt("item2.b")) - assertEquals("defaults.a", 2, resolved.getInt("defaults.a")) + assertEquals("defaults.a", 7, resolved.getInt("defaults.a")) } private val delayedMergeObjectResolveProblem6 = { From 597452cac0981acbbee354254954d75740727a13 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sat, 27 Dec 2014 21:13:17 -0500 Subject: [PATCH 14/14] Remove some dead code --- .../config/impl/ConfigConcatenation.java | 15 --- .../typesafe/config/impl/ResolveSource.java | 91 +------------------ 2 files changed, 1 insertion(+), 105 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java index 88f34ca9..dd69528f 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java @@ -282,19 +282,4 @@ final class ConfigConcatenation extends AbstractConfigValue implements Unmergeab p.render(sb, indent, atRoot, options); } } - - static List valuesFromPieces(ConfigOrigin origin, List pieces) { - List values = new ArrayList(pieces.size()); - for (Object p : pieces) { - if (p instanceof SubstitutionExpression) { - values.add(new ConfigReference(origin, (SubstitutionExpression) p)); - } else if (p instanceof String) { - values.add(new ConfigString(origin, (String) p)); - } else { - throw new ConfigException.BugOrBroken("Unexpected piece " + p); - } - } - - return values; - } } diff --git a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java index dd6a4d74..b192c305 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveSource.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveSource.java @@ -1,8 +1,5 @@ package com.typesafe.config.impl; -import java.util.Iterator; -import java.util.NoSuchElementException; - import com.typesafe.config.ConfigException; import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; @@ -161,19 +158,6 @@ final class ResolveSource { } } - ResolveSource popParent(Container parent) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace("popping parent " + parent); - if (parent == null) - throw new ConfigException.BugOrBroken("can't pop null parent"); - else if (pathFromRoot == null) - return this; - else if (pathFromRoot.head() != parent) - throw new ConfigException.BugOrBroken("parent was not pushed, can't pop: " + parent); - else - return new ResolveSource(root, pathFromRoot.tail()); - } - ResolveSource resetParents() { if (pathFromRoot == null) return this; @@ -271,27 +255,10 @@ final class ResolveSource { } // a persistent list - static final class Node implements Iterable { + static final class Node { final T value; final Node next; - @Override - public int hashCode() { - return value.hashCode() + 41 * (41 + next.hashCode()); - } - - @Override - public boolean equals(Object other) { - if (other instanceof Node) { - if (value == ((Node) other).value) - return true; - else - return value.equals(((Node) other).value); - } else { - return false; - } - } - Node(T value, Node next) { this.value = value; this.next = next; @@ -334,40 +301,6 @@ final class ResolveSource { } } - private static class NodeIterator implements Iterator { - Node current; - - NodeIterator(Node current) { - this.current = current; - } - - @Override - public T next() { - if (current == null) { - throw new NoSuchElementException(); - } else { - T result = current.value; - current = current.next; - return result; - } - } - - @Override - public boolean hasNext() { - return current != null; - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - } - - @Override - public Iterator iterator() { - return new NodeIterator(this); - } - @Override public String toString() { StringBuffer sb = new StringBuffer(); @@ -394,17 +327,6 @@ final class ResolveSource { this.pathFromRoot = pathFromRoot; } - ValueWithPath(AbstractConfigValue value) { - this(value, null); - } - - ValueWithPath addParent(Container parent) { - if (pathFromRoot == null) - return new ValueWithPath(value, new Node(parent)); - else - return new ValueWithPath(value, pathFromRoot.prepend(parent)); - } - @Override public String toString() { return "ValueWithPath(value=" + value + ", pathFromRoot=" + pathFromRoot + ")"; @@ -420,17 +342,6 @@ final class ResolveSource { this.pathFromRoot = pathFromRoot; } - ResultWithPath(ResolveResult result) { - this(result, null); - } - - ResultWithPath addParent(Container parent) { - if (pathFromRoot == null) - return new ResultWithPath(result, new Node(parent)); - else - return new ResultWithPath(result, pathFromRoot.prepend(parent)); - } - @Override public String toString() { return "ResultWithPath(result=" + result + ", pathFromRoot=" + pathFromRoot + ")";