From df3b3792da823ceabe92d030c4b087836612bcff Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 11 Nov 2011 23:51:41 -0500 Subject: [PATCH] make resolveSubstitutions() do nothing on already-resolved objects. This way if people resolve() gratuitously, there's no performance issue. Also, tracking resolved state gives us the option to complain about usage of unresolved objects. --- .../config/impl/AbstractConfigObject.java | 31 +++++++++++++------ .../config/impl/AbstractConfigValue.java | 4 +++ .../config/impl/ConfigDelayedMerge.java | 5 +++ .../config/impl/ConfigDelayedMergeObject.java | 11 ++++++- .../com/typesafe/config/impl/ConfigImpl.java | 2 +- .../config/impl/ConfigSubstitution.java | 5 +++ .../config/impl/DelegatingConfigObject.java | 18 ++++++----- .../java/com/typesafe/config/impl/Loader.java | 7 +++-- .../typesafe/config/impl/ResolveStatus.java | 23 ++++++++++++++ .../config/impl/RootConfigObject.java | 6 ++-- .../config/impl/SimpleConfigList.java | 18 ++++++++++- .../config/impl/SimpleConfigObject.java | 28 ++++++++++++++--- .../config/impl/SubstitutionResolver.java | 5 +++ 13 files changed, 134 insertions(+), 29 deletions(-) create mode 100644 src/main/java/com/typesafe/config/impl/ResolveStatus.java diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java index 06213e09..86cad897 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigObject.java @@ -129,13 +129,13 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements @Override AbstractConfigObject transformed(ConfigTransformer newTransformer) { if (newTransformer != transformer) - return newCopy(newTransformer); + return newCopy(newTransformer, resolveStatus()); else return this; } protected abstract AbstractConfigObject newCopy( - ConfigTransformer newTransformer); + ConfigTransformer newTransformer, ResolveStatus status); static private AbstractConfigValue resolve(AbstractConfigObject self, String pathExpression, @@ -199,6 +199,7 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements if (fallback.isEmpty()) { return this; // nothing to do } else { + boolean allResolved = true; Map merged = new HashMap(); Set allKeys = new HashSet(); allKeys.addAll(this.keySet()); @@ -206,15 +207,19 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements for (String key : allKeys) { AbstractConfigValue first = this.peek(key); AbstractConfigValue second = fallback.peek(key); + AbstractConfigValue kept; if (first == null) - merged.put(key, second); + kept = second; else if (second == null) - merged.put(key, first); + kept = first; else - merged.put(key, first.withFallback(second)); + kept = first.withFallback(second); + merged.put(key, kept); + if (kept.resolveStatus() == ResolveStatus.UNRESOLVED) + allResolved = false; } return new SimpleConfigObject(mergeOrigins(this, fallback), - merged); + merged, ResolveStatus.fromBoolean(allResolved)); } } else { // falling back to a non-object has no effect, we just override @@ -269,17 +274,22 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements AbstractConfigObject resolveSubstitutions(SubstitutionResolver resolver, int depth, boolean withFallbacks) { - Map changes = new HashMap(); + if (resolveStatus() == ResolveStatus.RESOLVED) + return this; + + Map changes = null; for (String k : keySet()) { AbstractConfigValue v = peek(k); AbstractConfigValue resolved = resolver.resolve(v, depth, withFallbacks); if (resolved != v) { + if (changes == null) + changes = new HashMap(); changes.put(k, resolved); } } - if (changes.isEmpty()) { - return this; + if (changes == null) { + return newCopy(transformer, ResolveStatus.RESOLVED); } else { Map resolved = new HashMap(); for (String k : keySet()) { @@ -289,7 +299,8 @@ abstract class AbstractConfigObject extends AbstractConfigValue implements resolved.put(k, peek(k)); } } - return new SimpleConfigObject(origin(), resolved); + return new SimpleConfigObject(origin(), transformer, resolved, + ResolveStatus.RESOLVED); } } diff --git a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java index aee30fa2..5b227f70 100644 --- a/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java +++ b/src/main/java/com/typesafe/config/impl/AbstractConfigValue.java @@ -34,6 +34,10 @@ abstract class AbstractConfigValue implements ConfigValue { return this; } + ResolveStatus resolveStatus() { + return ResolveStatus.RESOLVED; + } + @Override public AbstractConfigValue withFallback(ConfigValue other) { return this; diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java index 64b56beb..76d47940 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -85,6 +85,11 @@ final class ConfigDelayedMerge extends AbstractConfigValue implements return AbstractConfigObject.merge(toMerge); } + @Override + ResolveStatus resolveStatus() { + return ResolveStatus.UNRESOLVED; + } + @Override public AbstractConfigValue withFallback(ConfigValue other) { if (other instanceof AbstractConfigObject diff --git a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java index 3e355fbf..6c9fcad7 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java +++ b/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java @@ -64,7 +64,11 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements } @Override - public ConfigDelayedMergeObject newCopy(ConfigTransformer newTransformer) { + public ConfigDelayedMergeObject newCopy(ConfigTransformer newTransformer, + ResolveStatus status) { + if (status != resolveStatus()) + throw new ConfigException.BugOrBroken( + "attempt to create resolved ConfigDelayedMergeObject"); return new ConfigDelayedMergeObject(origin(), newTransformer, stack); } @@ -82,6 +86,11 @@ class ConfigDelayedMergeObject extends AbstractConfigObject implements } } + @Override + ResolveStatus resolveStatus() { + return ResolveStatus.UNRESOLVED; + } + @Override public ConfigDelayedMergeObject withFallback(ConfigValue other) { if (other instanceof AbstractConfigObject diff --git a/src/main/java/com/typesafe/config/impl/ConfigImpl.java b/src/main/java/com/typesafe/config/impl/ConfigImpl.java index e9701316..cb081f7b 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -128,6 +128,6 @@ public class ConfigImpl { new SimpleConfigOrigin("env var " + key), entry.getValue())); } return new SimpleConfigObject(new SimpleConfigOrigin("env variables"), - m); + m, ResolveStatus.RESOLVED); } } diff --git a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java index a1d74829..2cfe5064 100644 --- a/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java +++ b/src/main/java/com/typesafe/config/impl/ConfigSubstitution.java @@ -168,6 +168,11 @@ final class ConfigSubstitution extends AbstractConfigValue implements return resolved; } + @Override + ResolveStatus resolveStatus() { + return ResolveStatus.UNRESOLVED; + } + @Override protected boolean canEqual(Object other) { return other instanceof ConfigSubstitution; diff --git a/src/main/java/com/typesafe/config/impl/DelegatingConfigObject.java b/src/main/java/com/typesafe/config/impl/DelegatingConfigObject.java index 243b175e..801723c2 100644 --- a/src/main/java/com/typesafe/config/impl/DelegatingConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/DelegatingConfigObject.java @@ -16,16 +16,18 @@ abstract class DelegatingConfigObject extends AbstractConfigObject { } @Override - protected DelegatingConfigObject newCopy(ConfigTransformer newTransformer) { - AbstractConfigObject transformed = underlying - .transformed(newTransformer); - if (transformed != underlying) - return newCopy(transformed); - else - return this; + protected DelegatingConfigObject newCopy(ConfigTransformer newTransformer, + ResolveStatus newStatus) { + return newCopy(underlying, newTransformer, newStatus); } - abstract DelegatingConfigObject newCopy(AbstractConfigObject underlying); + abstract DelegatingConfigObject newCopy(AbstractConfigObject underlying, + ConfigTransformer newTransformer, ResolveStatus newStatus); + + @Override + ResolveStatus resolveStatus() { + return underlying.resolveStatus(); + } @Override public boolean containsKey(Object key) { diff --git a/src/main/java/com/typesafe/config/impl/Loader.java b/src/main/java/com/typesafe/config/impl/Loader.java index 7c40fc64..600c20d6 100644 --- a/src/main/java/com/typesafe/config/impl/Loader.java +++ b/src/main/java/com/typesafe/config/impl/Loader.java @@ -155,9 +155,12 @@ final class Loader { // provide to SimpleConfigObject, which is not allowed by // its contract, but since we know nobody has a ref to this // SimpleConfigObject yet we can get away with it. + // Also we assume here that any info based on the map that + // SimpleConfigObject computes and caches in its constructor + // will not change. Basically this is a bad hack. AbstractConfigObject o = new SimpleConfigObject( new SimpleConfigOrigin(originPrefix + " " + path), - scopes.get(path)); + scopes.get(path), ResolveStatus.RESOLVED); String basename = lastElement(path); parent.put(basename, o); } @@ -171,6 +174,6 @@ final class Loader { // return root config object return new SimpleConfigObject(new SimpleConfigOrigin(originPrefix), - root); + root, ResolveStatus.RESOLVED); } } diff --git a/src/main/java/com/typesafe/config/impl/ResolveStatus.java b/src/main/java/com/typesafe/config/impl/ResolveStatus.java new file mode 100644 index 00000000..3b6a7535 --- /dev/null +++ b/src/main/java/com/typesafe/config/impl/ResolveStatus.java @@ -0,0 +1,23 @@ +package com.typesafe.config.impl; + +import java.util.Collection; + +/** + * Status of substitution resolution. + */ +enum ResolveStatus { + UNRESOLVED, RESOLVED; + + final static ResolveStatus fromValues( + Collection values) { + for (AbstractConfigValue v : values) { + if (v.resolveStatus() == ResolveStatus.UNRESOLVED) + return ResolveStatus.UNRESOLVED; + } + return ResolveStatus.RESOLVED; + } + + final static ResolveStatus fromBoolean(boolean resolved) { + return resolved ? ResolveStatus.RESOLVED : ResolveStatus.UNRESOLVED; + } +} diff --git a/src/main/java/com/typesafe/config/impl/RootConfigObject.java b/src/main/java/com/typesafe/config/impl/RootConfigObject.java index 9bb5194c..f6a7ff6b 100644 --- a/src/main/java/com/typesafe/config/impl/RootConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/RootConfigObject.java @@ -16,8 +16,10 @@ final class RootConfigObject extends DelegatingConfigObject implements } @Override - public RootConfigObject newCopy(AbstractConfigObject underlying) { - return new RootConfigObject(underlying); + public RootConfigObject newCopy(AbstractConfigObject underlying, + ConfigTransformer newTransformer, ResolveStatus newStatus) { + return new RootConfigObject(underlying.newCopy(newTransformer, + newStatus)); } @Override diff --git a/src/main/java/com/typesafe/config/impl/SimpleConfigList.java b/src/main/java/com/typesafe/config/impl/SimpleConfigList.java index 00f331af..54b463a3 100644 --- a/src/main/java/com/typesafe/config/impl/SimpleConfigList.java +++ b/src/main/java/com/typesafe/config/impl/SimpleConfigList.java @@ -15,10 +15,17 @@ import com.typesafe.config.ConfigValueType; final class SimpleConfigList extends AbstractConfigValue implements ConfigList { final private List value; + final private boolean resolved; SimpleConfigList(ConfigOrigin origin, List value) { + this(origin, value, ResolveStatus.fromValues(value)); + } + + SimpleConfigList(ConfigOrigin origin, List value, + ResolveStatus status) { super(origin); this.value = value; + this.resolved = status == ResolveStatus.RESOLVED; } @Override @@ -35,9 +42,17 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { return list; } + @Override + ResolveStatus resolveStatus() { + return ResolveStatus.fromBoolean(resolved); + } + @Override SimpleConfigList resolveSubstitutions(SubstitutionResolver resolver, int depth, boolean withFallbacks) { + if (resolved) + return this; + // lazy-create for optimization List changed = null; int i = 0; @@ -66,7 +81,8 @@ final class SimpleConfigList extends AbstractConfigValue implements ConfigList { if (changed.size() != value.size()) throw new ConfigException.BugOrBroken( "substituted list's size doesn't match"); - return new SimpleConfigList(origin(), changed); + return new SimpleConfigList(origin(), changed, + ResolveStatus.RESOLVED); } else { return this; } diff --git a/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java b/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java index dc38e7fd..9ba462ff 100644 --- a/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -19,19 +19,32 @@ final class SimpleConfigObject extends AbstractConfigObject { // this map should never be modified - assume immutable final private Map value; + final private boolean resolved; SimpleConfigObject(ConfigOrigin origin, ConfigTransformer transformer, - Map value) { + Map value, ResolveStatus status) { super(origin, transformer); if (value == null) throw new ConfigException.BugOrBroken( "creating config object with null map"); this.value = value; + this.resolved = status == ResolveStatus.RESOLVED; + } + + SimpleConfigObject(ConfigOrigin origin, ConfigTransformer transformer, + Map value) { + this(origin, transformer, value, ResolveStatus.fromValues(value + .values())); + } + + SimpleConfigObject(ConfigOrigin origin, + Map value, ResolveStatus status) { + this(origin, ConfigImpl.defaultConfigTransformer(), value, status); } SimpleConfigObject(ConfigOrigin origin, Map value) { - this(origin, ConfigImpl.defaultConfigTransformer(), value); + this(origin, value, ResolveStatus.fromValues(value.values())); } @Override @@ -40,8 +53,15 @@ final class SimpleConfigObject extends AbstractConfigObject { } @Override - public SimpleConfigObject newCopy(ConfigTransformer newTransformer) { - return new SimpleConfigObject(origin(), newTransformer, value); + public SimpleConfigObject newCopy(ConfigTransformer newTransformer, + ResolveStatus newStatus) { + return new SimpleConfigObject(origin(), newTransformer, value, + newStatus); + } + + @Override + ResolveStatus resolveStatus() { + return ResolveStatus.fromBoolean(resolved); } @Override diff --git a/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java b/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java index feb99735..fd321143 100644 --- a/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java +++ b/src/main/java/com/typesafe/config/impl/SubstitutionResolver.java @@ -3,6 +3,8 @@ package com.typesafe.config.impl; import java.util.IdentityHashMap; import java.util.Map; +import com.typesafe.config.ConfigException; + /** * This exists because we have to memoize resolved substitutions as we go * through the config tree; otherwise we could end up creating multiple copies @@ -26,6 +28,9 @@ final class SubstitutionResolver { AbstractConfigValue resolved = original.resolveSubstitutions(this, depth, withFallbacks); + if (resolved.resolveStatus() != ResolveStatus.RESOLVED) + throw new ConfigException.BugOrBroken( + "resolveSubstitutions() did not give us a resolved object"); memos.put(original, resolved); return resolved; }