Skip to content

Commit

Permalink
Rewrite substitution resolver, use explicit immutable ResolveSource
Browse files Browse the repository at this point in the history
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
  • Loading branch information
havocp committed Jul 10, 2014
1 parent fdce50f commit 0a20b9a
Show file tree
Hide file tree
Showing 17 changed files with 751 additions and 351 deletions.
1 change: 1 addition & 0 deletions config/.gitignore
@@ -0,0 +1 @@
/bin
Expand Up @@ -17,7 +17,7 @@
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;

Expand Down Expand Up @@ -56,7 +56,8 @@ public AbstractConfigObject toFallbackValue() {
/**
* 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
Expand All @@ -78,67 +79,34 @@ protected final AbstractConfigValue peekAssumingResolved(String key, Path origin
* 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) {
Expand Down Expand Up @@ -209,7 +177,8 @@ static ConfigOrigin mergeOrigins(AbstractConfigObject... stack) {
}

@Override
abstract AbstractConfigObject resolveSubstitutions(ResolveContext context) throws NotPossibleToResolve;
abstract AbstractConfigObject resolveSubstitutions(ResolveContext context, ResolveSource source)
throws NotPossibleToResolve;

@Override
abstract AbstractConfigObject relativized(final Path prefix);
Expand Down
Expand Up @@ -68,9 +68,11 @@ String traceString() {
*
* @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;
}
Expand All @@ -79,6 +81,38 @@ ResolveStatus resolveStatus() {
return ResolveStatus.RESOLVED;
}

protected static List<AbstractConfigValue> replaceChildInList(List<AbstractConfigValue> 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<AbstractConfigValue> newStack = new ArrayList<AbstractConfigValue>(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<AbstractConfigValue> 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
Expand Down
Expand Up @@ -22,7 +22,7 @@
* 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<AbstractConfigValue> pieces;

Expand Down Expand Up @@ -170,7 +170,7 @@ static AbstractConfigValue concatenate(List<AbstractConfigValue> pieces) {
}

@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:");
Expand All @@ -181,11 +181,16 @@ AbstractConfigValue resolveSubstitutions(ResolveContext context) throws NotPossi
}
}

// 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<AbstractConfigValue> resolved = new ArrayList<AbstractConfigValue>(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) {
Expand Down Expand Up @@ -215,6 +220,20 @@ ResolveStatus resolveStatus() {
return ResolveStatus.UNRESOLVED;
}

@Override
public ConfigConcatenation replaceChild(AbstractConfigValue child, AbstractConfigValue replacement) {
List<AbstractConfigValue> 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
Expand Down

0 comments on commit 0a20b9a

Please sign in to comment.