To detect cycles, track visited MemoKey not visited nodes

Otherwise we can get a fake cycle when we try to do a partial
resolve on a node we are currently doing a full resolve on.
This commit is contained in:
Havoc Pennington 2012-03-28 23:07:00 -04:00
parent db85e5ec8a
commit 5187a7ad4d
11 changed files with 75 additions and 73 deletions

View File

@ -92,7 +92,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
* @throws NotPossibleToResolve
*/
protected AbstractConfigValue peekPath(Path path, SubstitutionResolver resolver,
Set<ConfigSubstitution> traversed, ConfigResolveOptions options)
Set<MemoKey> 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.<ConfigSubstitution> emptySet(), null);
return peekPath(this, path, null, Collections.<MemoKey> 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<ConfigSubstitution> traversed,
ConfigResolveOptions options)
SubstitutionResolver resolver, Set<MemoKey> 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.<ConfigSubstitution> emptySet(), null);
Collections.<MemoKey> emptySet(), null);
} else {
return null;
}
@ -225,8 +224,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements
@Override
abstract AbstractConfigObject resolveSubstitutions(final SubstitutionResolver resolver,
Set<ConfigSubstitution> traversed, ConfigResolveOptions options,
Path restrictToChildOrNull)
Set<MemoKey> traversed, ConfigResolveOptions options, Path restrictToChildOrNull)
throws NotPossibleToResolve, NeedsFullResolve;
@Override

View File

@ -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<ConfigSubstitution> traversed, ConfigResolveOptions options,
Path restrictToChildOrNull) throws NotPossibleToResolve,
AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set<MemoKey> traversed,
ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve,
NeedsFullResolve {
return this;
}

View File

@ -65,17 +65,15 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements
}
@Override
AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver,
Set<ConfigSubstitution> traversed, ConfigResolveOptions options,
Path restrictToChildOrNull) throws NotPossibleToResolve,
AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set<MemoKey> 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<AbstractConfigValue> stack,
SubstitutionResolver resolver, Set<ConfigSubstitution> traversed,
ConfigResolveOptions options,
SubstitutionResolver resolver, Set<MemoKey> 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

View File

@ -62,9 +62,8 @@ final class ConfigDelayedMergeObject extends AbstractConfigObject implements
@Override
AbstractConfigObject resolveSubstitutions(SubstitutionResolver resolver,
Set<ConfigSubstitution> traversed, ConfigResolveOptions options,
Path restrictToChildOrNull) throws NotPossibleToResolve,
NeedsFullResolve {
Set<MemoKey> traversed, ConfigResolveOptions options, Path restrictToChildOrNull)
throws NotPossibleToResolve, NeedsFullResolve {
AbstractConfigValue merged = ConfigDelayedMerge.resolveSubstitutions(stack, resolver,
traversed, options, restrictToChildOrNull);
if (merged instanceof AbstractConfigObject) {

View File

@ -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<ConfigSubstitution> traversed, ConfigResolveOptions options)
Path subst, Set<MemoKey> 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<ConfigSubstitution> traversed,
ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve,
SubstitutionExpression subst, Set<MemoKey> 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<ConfigSubstitution> traversed,
private ConfigValue resolve(SubstitutionResolver resolver, Set<MemoKey> 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<ConfigSubstitution> traversed, ConfigResolveOptions options,
Path restrictToChildOrNull) throws NotPossibleToResolve {
AbstractConfigValue resolveSubstitutions(SubstitutionResolver resolver, Set<MemoKey> traversed,
ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve {
AbstractConfigValue resolved = (AbstractConfigValue) resolve(resolver, traversed, options,
restrictToChildOrNull);
return resolved;

View File

@ -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;
}
}
}

View File

@ -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.<ConfigSubstitution> emptySet(), null);
peeked = object.peekPath(path, null, Collections.<MemoKey> emptySet(), null);
} catch (NotPossibleToResolve e) {
throw e.exportException(origin(), pathExpression);
} catch (NeedsFullResolve e) {

View File

@ -106,7 +106,7 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList {
@Override
SimpleConfigList resolveSubstitutions(final SubstitutionResolver resolver,
final Set<ConfigSubstitution> traversed, final ConfigResolveOptions options,
final Set<MemoKey> traversed, final ConfigResolveOptions options,
Path restrictToChildOrNull)
throws NotPossibleToResolve, NeedsFullResolve {
if (resolved)

View File

@ -261,7 +261,7 @@ final class SimpleConfigObject extends AbstractConfigObject {
@Override
AbstractConfigObject resolveSubstitutions(final SubstitutionResolver resolver,
final Set<ConfigSubstitution> traversed, final ConfigResolveOptions options,
final Set<MemoKey> traversed, final ConfigResolveOptions options,
final Path restrictToChildOrNull)
throws NotPossibleToResolve, NeedsFullResolve {
if (resolveStatus() == ResolveStatus.RESOLVED)

View File

@ -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<MemoKey, AbstractConfigValue>();
}
AbstractConfigValue resolve(AbstractConfigValue original, Set<ConfigSubstitution> traversed,
AbstractConfigValue resolve(AbstractConfigValue original, Set<MemoKey> traversed,
ConfigResolveOptions options, Path restrictToChildOrNull) throws NotPossibleToResolve,
NeedsFullResolve {
@ -122,8 +84,10 @@ final class SubstitutionResolver {
return this.root;
}
private static Set<ConfigSubstitution> newTraversedSet() {
return Collections.newSetFromMap(new IdentityHashMap<ConfigSubstitution, Boolean>());
private static Set<MemoKey> newTraversedSet() {
// using LinkedHashSet just to show the order of traversal
// in error messages.
return new LinkedHashSet<MemoKey>();
}
static AbstractConfigValue resolve(AbstractConfigValue value, AbstractConfigObject root,

View File

@ -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 {