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 cec79684..b90f7845 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -92,7 +92,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements * @throws NotPossibleToResolve */ protected AbstractConfigValue peekPath(Path path, SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options) + Set traversed, ConfigResolveOptions options) throws NotPossibleToResolve, NeedsFullResolve { return peekPath(this, path, resolver, traversed, options); } @@ -103,7 +103,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements */ AbstractConfigValue peekPathWithExternalExceptions(Path path) { try { - return peekPath(this, path, null, Collections. emptySet(), null); + return peekPath(this, path, null, Collections. emptySet(), null); } catch (NotPossibleToResolve e) { throw e.exportException(origin(), path.render()); } catch (NeedsFullResolve e) { @@ -116,8 +116,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements // 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, - SubstitutionResolver resolver, Set traversed, - ConfigResolveOptions options) + SubstitutionResolver resolver, Set traversed, ConfigResolveOptions options) throws NotPossibleToResolve, NeedsFullResolve { if (resolver != null) { // walk down through the path resolving only things along that path, @@ -142,7 +141,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements } else { if (v instanceof AbstractConfigObject) { return peekPath((AbstractConfigObject) v, next, null, - Collections. emptySet(), null); + Collections. emptySet(), null); } else { return null; } @@ -225,8 +224,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements @Override abstract AbstractConfigObject resolveSubstitutions(final SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options, - Path restrictToChildOrNull) + Set traversed, ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve; @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 54f602cf..17412a7c 100644 --- a/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/config/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -110,9 +110,8 @@ abstract class AbstractConfigValue implements ConfigValue, MergeableValue, Seria * if non-null, only recurse into this child path * @return a new value if there were changes, or this if no changes */ - AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options, - Path restrictToChildOrNull) throws NotPossibleToResolve, + AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set traversed, + ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { return this; } 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 58c026f4..72d0eeb0 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -65,17 +65,15 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements } @Override - AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options, - Path restrictToChildOrNull) throws NotPossibleToResolve, + AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set traversed, + ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { return resolveSubstitutions(stack, resolver, traversed, options, restrictToChildOrNull); } // static method also used by ConfigDelayedMergeObject static AbstractConfigValue resolveSubstitutions(List stack, - SubstitutionResolver resolver, Set traversed, - ConfigResolveOptions options, + SubstitutionResolver resolver, Set traversed, ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { // to resolve substitutions, we need to recursively resolve // the stack of stuff to merge, and merge the stack so 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 baf91c78..8393f9a9 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -62,9 +62,8 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements @Override AbstractConfigObject resolveSubstitutions(SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options, - Path restrictToChildOrNull) throws NotPossibleToResolve, - NeedsFullResolve { + Set traversed, ConfigResolveOptions options, Path restrictToChildOrNull) + throws NotPossibleToResolve, NeedsFullResolve { AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(stack, resolver, traversed, options, restrictToChildOrNull); if (merged instanceof AbstractConfigObject) { diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java index c0723469..f74d3776 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -127,7 +127,7 @@ final class ConfigSubstitution extends AbstractConfigValue implements private static AbstractConfigValue findInObject(AbstractConfigObject root, SubstitutionResolver resolver, /* null if we should not have refs */ - Path subst, Set traversed, ConfigResolveOptions options) + Path subst, Set traversed, ConfigResolveOptions options) throws NotPossibleToResolve, NeedsFullResolve { AbstractConfigValue result = root.peekPath(subst, resolver, traversed, options); @@ -136,13 +136,14 @@ final class ConfigSubstitution extends AbstractConfigValue implements } private AbstractConfigValue resolve(SubstitutionResolver resolver, - SubstitutionExpression subst, Set traversed, - ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, + SubstitutionExpression subst, Set traversed, ConfigResolveOptions options, + Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { - if (traversed.contains(this)) + MemoKey key = new MemoKey(this, restrictToChildOrNull); + if (traversed.contains(key)) throw new SelfReferential(origin(), subst.path().render()); - traversed.add(this); + traversed.add(key); try { @@ -175,11 +176,11 @@ final class ConfigSubstitution extends AbstractConfigValue implements return result; } finally { - traversed.remove(this); + traversed.remove(key); } } - private ConfigValue resolve(SubstitutionResolver resolver, Set traversed, + private ConfigValue resolve(SubstitutionResolver resolver, Set traversed, ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve { if (pieces.size() > 1) { // need to concat everything into a string @@ -240,9 +241,8 @@ final class ConfigSubstitution extends AbstractConfigValue implements } @Override - AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, - Set traversed, ConfigResolveOptions options, - Path restrictToChildOrNull) throws NotPossibleToResolve { + AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set traversed, + ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve { AbstractConfigValue resolved = (AbstractConfigValue) resolve(resolver, traversed, options, restrictToChildOrNull); return resolved; diff --git a/config/src/main/java/com/typesafe/config/impl/MemoKey.java b/config/src/main/java/com/typesafe/config/impl/MemoKey.java new file mode 100644 index 00000000..7237234f --- /dev/null +++ b/config/src/main/java/com/typesafe/config/impl/MemoKey.java @@ -0,0 +1,39 @@ +package com.typesafe.config.impl; + +/** The key used to memoize already-traversed nodes when resolving substitutions */ +final class MemoKey { + MemoKey(AbstractConfigValue value, Path restrictToChildOrNull) { + this.value = value; + this.restrictToChildOrNull = restrictToChildOrNull; + } + + final private AbstractConfigValue value; + final private Path restrictToChildOrNull; + + @Override + public final int hashCode() { + int h = System.identityHashCode(value); + if (restrictToChildOrNull != null) { + return h + 41 * (41 + restrictToChildOrNull.hashCode()); + } else { + return h; + } + } + + @Override + public final boolean equals(Object other) { + if (other instanceof MemoKey) { + MemoKey o = (MemoKey) other; + if (o.value != this.value) + return false; + else if (o.restrictToChildOrNull == this.restrictToChildOrNull) + return true; + else if (o.restrictToChildOrNull == null || this.restrictToChildOrNull == null) + return false; + else + return o.restrictToChildOrNull.equals(this.restrictToChildOrNull); + } else { + return false; + } + } +} diff --git a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java index 4dbb69fe..c7271231 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfig.java @@ -74,7 +74,7 @@ final class SimpleConfig implements Config, MergeableValue, Serializable { Path path = Path.newPath(pathExpression); ConfigValue peeked; try { - peeked = object.peekPath(path, null, Collections. emptySet(), null); + peeked = object.peekPath(path, null, Collections. emptySet(), null); } catch (NotPossibleToResolve e) { throw e.exportException(origin(), pathExpression); } catch (NeedsFullResolve e) { 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 e5e7a76e..aca3bf72 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -106,7 +106,7 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { @Override SimpleConfigList resolveSubstitutions(final SubstitutionResolver resolver, - final Set traversed, final ConfigResolveOptions options, + final Set traversed, final ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { if (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 b76c1ff0..aad2b8d4 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -261,7 +261,7 @@ final class SimpleConfigObject extends AbstractConfigObject { @Override AbstractConfigObject resolveSubstitutions(final SubstitutionResolver resolver, - final Set traversed, final ConfigResolveOptions options, + final Set traversed, final ConfigResolveOptions options, final Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { if (resolveStatus() == ResolveStatus.RESOLVED) diff --git a/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java b/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java index baa015f7..d3a975c2 100644 --- a/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java +++ b/config/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java @@ -3,9 +3,8 @@ */ package com.typesafe.config.impl; -import java.util.Collections; import java.util.HashMap; -import java.util.IdentityHashMap; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; @@ -20,43 +19,6 @@ import com.typesafe.config.impl.AbstractConfigValue.NotPossibleToResolve; * of values or whole trees of values as we follow chains of substitutions. */ final class SubstitutionResolver { - private final class MemoKey { - MemoKey(AbstractConfigValue value, Path restrictToChildOrNull) { - this.value = value; - this.restrictToChildOrNull = restrictToChildOrNull; - } - - final private AbstractConfigValue value; - final private Path restrictToChildOrNull; - - @Override - public final int hashCode() { - int h = System.identityHashCode(value); - if (restrictToChildOrNull != null) { - return h + 41 * (41 + restrictToChildOrNull.hashCode()); - } else { - return h; - } - } - - @Override - public final boolean equals(Object other) { - if (other instanceof MemoKey) { - MemoKey o = (MemoKey) other; - if (o.value != this.value) - return false; - else if (o.restrictToChildOrNull == this.restrictToChildOrNull) - return true; - else if (o.restrictToChildOrNull == null || this.restrictToChildOrNull == null) - return false; - else - return o.restrictToChildOrNull.equals(this.restrictToChildOrNull); - } else { - return false; - } - } - } - final private AbstractConfigObject root; // note that we can resolve things to undefined (represented as Java null, // rather than ConfigNull) so this map can have null values. @@ -67,7 +29,7 @@ final class SubstitutionResolver { this.memos = new HashMap(); } - AbstractConfigValue resolve(AbstractConfigValue original, Set traversed, + AbstractConfigValue resolve(AbstractConfigValue original, Set traversed, ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve, NeedsFullResolve { @@ -122,8 +84,10 @@ final class SubstitutionResolver { return this.root; } - private static Set newTraversedSet() { - return Collections.newSetFromMap(new IdentityHashMap()); + private static Set newTraversedSet() { + // using LinkedHashSet just to show the order of traversal + // in error messages. + return new LinkedHashSet(); } static AbstractConfigValue resolve(AbstractConfigValue value, AbstractConfigObject root, 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 ec8aa2a1..440d9ca5 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -305,6 +305,11 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(43, resolved.getInt("item2.b.c")) } + // this case has to traverse ${defaults} twice, once + // trying to resolve it all and then as part of that + // trying a partial resolve of only b.c + // thus a simple cycle-detector would get confused + // and think that defaults was a cycle. private val delayedMergeObjectResolveProblem3 = { parseObject(""" defaults {