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 ae0bae1d..00c58df5 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigConcatenation.java @@ -193,10 +193,12 @@ ResolveResult resolveSubstitutions(ResolveContext } } - // 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); + // Pieces can contain containers with unresolved substitutions (e.g. a + // delayed merge inside a list), and resolving those replaces them via + // ResolveSource.replaceWithinCurrentParent(); for the replacement to + // walk back up to the root, this concatenation must be on the parent + // chain. + ResolveSource sourceWithParent = source.pushParent(this); ResolveContext newContext = context; List resolved = new ArrayList(pieces.size()); 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 68fd3997..792d44ac 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -81,6 +81,7 @@ static ResolveResult resolveSubstitutions(Replace ResolveContext newContext = context; int count = 0; AbstractConfigValue merged = null; + AbstractConfigValue wasMergeStackPiece = context.mergeStackPiece(); for (AbstractConfigValue end : stack) { // Per the HOCON spec, a substitution hidden by a value that // cannot be merged with it is never evaluated. If merged already @@ -146,6 +147,11 @@ && allKeysShadowed((SimpleConfigObject) end, (AbstractConfigObject) merged)) { count += 1; continue; } + + // mark the piece so self-referential fields directly inside + // it are dropped (looking back at the pieces below) when a + // lookup re-enters them while this piece is still resolving + newContext = newContext.withMergeStackPiece(end); } if (ConfigImpl.traceSubstitutionsEnabled()) { @@ -157,7 +163,7 @@ && allKeysShadowed((SimpleConfigObject) end, (AbstractConfigObject) merged)) { + " against " + sourceForEnd + " endWasRemoved=" + (source != sourceForEnd)); ResolveResult result = newContext.resolve(end, sourceForEnd); AbstractConfigValue resolvedEnd = result.value; - newContext = result.context; + newContext = result.context.withMergeStackPiece(wasMergeStackPiece); if (resolvedEnd != null) { if (merged == 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 bb59af94..5b1c9462 100644 --- a/config/src/main/java/com/typesafe/config/impl/ResolveContext.java +++ b/config/src/main/java/com/typesafe/config/impl/ResolveContext.java @@ -27,8 +27,14 @@ final class ResolveContext { final private Set cycleMarkers; + // the delayed merge stack piece currently being resolved, or null. A + // self-referential field directly inside the piece is dropped in favor of + // earlier pieces when a lookup re-enters it while the piece resolves. + final private AbstractConfigValue mergeStackPiece; + ResolveContext(ResolveMemos memos, ConfigResolveOptions options, Path restrictToChild, - List resolveStack, Set cycleMarkers) { + List resolveStack, Set cycleMarkers, + AbstractConfigValue mergeStackPiece) { this.memos = memos; this.options = options; this.restrictToChild = restrictToChild; @@ -38,6 +44,7 @@ final class ResolveContext { // up shared between multiple ResolveContext. this.resolveStack = resolveStack; this.cycleMarkers = cycleMarkers; + this.mergeStackPiece = mergeStackPiece; } private static Set newCycleMarkers() { @@ -47,7 +54,8 @@ private static Set newCycleMarkers() { 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(), newCycleMarkers()); + this(new ResolveMemos(), options, restrictToChild, new ArrayList(), newCycleMarkers(), + null); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "ResolveContext restrict to child " + restrictToChild); } @@ -60,7 +68,7 @@ ResolveContext addCycleMarker(AbstractConfigValue value) { Set copy = newCycleMarkers(); copy.addAll(cycleMarkers); copy.add(value); - return new ResolveContext(memos, options, restrictToChild, resolveStack, copy); + return new ResolveContext(memos, options, restrictToChild, resolveStack, copy, mergeStackPiece); } ResolveContext removeCycleMarker(AbstractConfigValue value) { @@ -70,12 +78,12 @@ ResolveContext removeCycleMarker(AbstractConfigValue value) { Set copy = newCycleMarkers(); copy.addAll(cycleMarkers); copy.remove(value); - return new ResolveContext(memos, options, restrictToChild, resolveStack, copy); + return new ResolveContext(memos, options, restrictToChild, resolveStack, copy, mergeStackPiece); } private ResolveContext memoize(MemoKey key, AbstractConfigValue value) { ResolveMemos changed = memos.put(key, value); - return new ResolveContext(changed, options, restrictToChild, resolveStack, cycleMarkers); + return new ResolveContext(changed, options, restrictToChild, resolveStack, cycleMarkers, mergeStackPiece); } ConfigResolveOptions options() { @@ -95,13 +103,35 @@ ResolveContext restrict(Path restrictTo) { if (restrictTo == restrictToChild) return this; else - return new ResolveContext(memos, options, restrictTo, resolveStack, cycleMarkers); + return new ResolveContext(memos, options, restrictTo, resolveStack, cycleMarkers, mergeStackPiece); } ResolveContext unrestricted() { return restrict(null); } + ResolveContext withMergeStackPiece(AbstractConfigValue mergeStackPiece) { + if (mergeStackPiece == this.mergeStackPiece) + return this; + else + return new ResolveContext(memos, options, restrictToChild, resolveStack, cycleMarkers, mergeStackPiece); + } + + AbstractConfigValue mergeStackPiece() { + return mergeStackPiece; + } + + // identity check whether the value is already being resolved somewhere + // else on the resolve stack; the current resolution itself is always on + // top of the stack, so it is not counted + boolean isResolvingElsewhere(AbstractConfigValue value) { + for (int i = 0; i < resolveStack.size() - 1; i++) { + if (resolveStack.get(i) == value) + return true; + } + return false; + } + String traceString() { String separator = ", "; StringBuilder sb = new StringBuilder(); @@ -121,7 +151,7 @@ private ResolveContext pushTrace(AbstractConfigValue value) { ConfigImpl.trace(depth(), "pushing trace " + value); List copy = new ArrayList(resolveStack); copy.add(value); - return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers); + return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers, mergeStackPiece); } ResolveContext popTrace() { @@ -129,7 +159,7 @@ ResolveContext popTrace() { AbstractConfigValue old = copy.remove(resolveStack.size() - 1); if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth() - 1, "popped trace " + old); - return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers); + return new ResolveContext(memos, options, restrictToChild, copy, cycleMarkers, mergeStackPiece); } int depth() { @@ -189,6 +219,12 @@ private ResolveResult realResolve(AbstractConfigV ResolveContext withMemo = result.context; + // a resolution re-entered while the same value is still being + // resolved further out depends on that in-flight state (for + // example a self-referential field looking back inside a merge + // stack), so its result must not be cached + boolean reentering = mergeStackPiece != null && isResolvingElsewhere(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 @@ -197,7 +233,8 @@ private ResolveResult realResolve(AbstractConfigV if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); - withMemo = withMemo.memoize(fullKey, resolved); + if (!reentering) + 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 @@ -210,12 +247,14 @@ private ResolveResult realResolve(AbstractConfigV if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + restrictedKey + " result " + resolved); - withMemo = withMemo.memoize(restrictedKey, resolved); + if (!reentering) + withMemo = withMemo.memoize(restrictedKey, resolved); } else if (options().getAllowUnresolved()) { if (ConfigImpl.traceSubstitutionsEnabled()) ConfigImpl.trace(depth(), "caching " + fullKey + " result " + resolved); - withMemo = withMemo.memoize(fullKey, resolved); + if (!reentering) + withMemo = withMemo.memoize(fullKey, resolved); } else { throw new ConfigException.BugOrBroken( "resolveSubstitutions() did not give us a resolved object"); 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 b77460e3..6c191182 100644 --- a/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java +++ b/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java @@ -435,15 +435,25 @@ private SimpleConfigObject modifyMayThrow(Modifier modifier) throws Exception { } } + // HOCON.md: a self-referential field has a substitution, or a value + // concatenation containing a substitution, as its value + private static boolean potentiallySelfReferential(AbstractConfigValue v) { + return v instanceof ConfigReference || v instanceof ConfigConcatenation; + } + private static final class ResolveModifier implements Modifier { final Path originalRestrict; + // Allow a self-referential leaf to look back within a merge piece, + // including when nested in one of its object fields. + final boolean dropInFlightChildren; ResolveContext context; final ResolveSource source; - ResolveModifier(ResolveContext context, ResolveSource source) { + ResolveModifier(ResolveContext context, ResolveSource source, boolean dropInFlightChildren) { this.context = context; this.source = source; + this.dropInFlightChildren = dropInFlightChildren; originalRestrict = context.restrictToChild(); } @@ -458,7 +468,12 @@ public AbstractConfigValue modifyChildMayThrow(String key, AbstractConfigValue v context = result.context.unrestricted().restrict(originalRestrict); return result.value; } else { - // we don't want to resolve the leaf child. + // Drop an in-flight self-referential leaf to look back through the merge. + // Object and array cycles remain unbreakable under HOCON.md. + if (dropInFlightChildren && potentiallySelfReferential(v) + && context.isResolvingElsewhere(v)) { + return null; + } return v; } } else { @@ -484,7 +499,12 @@ ResolveResult resolveSubstitutions(ResolveContex final ResolveSource sourceWithParent = source.pushParent(this); try { - ResolveModifier modifier = new ResolveModifier(context, sourceWithParent); + boolean inMergeStackPiece = context.mergeStackPiece() == this; + for (ResolveSource.Node node = sourceWithParent.pathFromRoot; + !inMergeStackPiece && node != null; node = node.tail()) { + inMergeStackPiece = node.head() == context.mergeStackPiece(); + } + ResolveModifier modifier = new ResolveModifier(context, sourceWithParent, inMergeStackPiece); AbstractConfigValue value = modifyMayThrow(modifier); return ResolveResult.make(modifier.context, value).asObjectResult(); 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 76e9709b..9e64b428 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConcatenationTest.scala @@ -656,4 +656,49 @@ class ConcatenationTest extends TestUtils { parseConfig("""foo = [1], bar = [2], x = ${foo}" "${bar}""").resolve() } } + + // https://github.com/lightbend/config/issues/751 + // A delayed merge (a duplicate key whose later value is a substitution) + // inside a list that is a piece of a value concatenation used to throw + // BugOrBroken "SimpleConfigObject.replaceChild did not find ...": the + // concatenation was not on the substitution source's parent chain, so the + // resolver walked past it and could not find the list in the parent. + @Test + def delayedMergeInConcatenatedListPiece(): Unit = { + val conf = parseConfig(""" + |tail = [ 2 ] + |front = [ + | { + | foo = 1 + | foo = ${?bar} + | } + |] ${tail} + |""".stripMargin).resolve() + + val front = conf.getList("front") + assertEquals(2, front.size()) + assertEquals(1, front.get(0).asInstanceOf[AbstractConfigObject].toConfig.getInt("foo")) + assertEquals(2, front.get(1).unwrapped()) + } + + // Same shape as delayedMergeInConcatenatedListPiece, but the substitution + // is strict and defined, so it takes part in the delayed merge. + @Test + def definedSubstitutionInConcatenatedListPiece(): Unit = { + val conf = parseConfig(""" + |bar = 7 + |tail = [ 2 ] + |front = [ + | { + | foo = 1 + | foo = ${bar} + | } + |] ${tail} + |""".stripMargin).resolve() + + val front = conf.getList("front") + assertEquals(2, front.size()) + assertEquals(7, front.get(0).asInstanceOf[AbstractConfigObject].toConfig.getInt("foo")) + assertEquals(2, front.get(1).unwrapped()) + } } 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 9ef25883..01fc41bf 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -318,6 +318,64 @@ class ConfigSubstitutionTest extends TestUtils { assertEquals(42, resolved.getInt("a.cycle")) } + // issue #728: a self-referential list concatenation in a field of an + // object merged over a ${d} substitution must look back at the earlier + // definition of the field instead of reporting a cycle + @Test + def selfReferentialListConcatenationThroughMerge() { + val resolved = resolve(parseObject("""d { x = [] }, c : ${d}, c { x : ${c.x}[1, 2] }""")) + assertEquals(Seq(1, 2), resolved.getList("c.x").asScala.map(_.unwrapped())) + } + + @Test + def selfReferentialListConcatenationThroughMergeSingleElement() { + val resolved = resolve(parseObject("""d { x = [] }, c : ${d}, c { x : ${c.x}[1] }""")) + assertEquals(Seq(1), resolved.getList("c.x").asScala.map(_.unwrapped())) + } + + @Test + def substitutionThroughMergeStillLooksForward() { + // a sibling field of a merge piece refers to a key the piece itself + // defines; it must keep seeing the piece's final value + val resolved = resolve(parseObject("""d { x = 1 }, c : ${d}, c { x = 2, y = ${c.x} }""")) + assertEquals(2, resolved.getInt("c.y")) + } + + @Test + def throwOnSelfReferentialCycleInsideObject() { + val e = intercept[ConfigException.UnresolvedSubstitution] { + resolve(parseObject("""a { b = ${a.c}, c = ${a.b} }""")) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle")) + } + + @Test + def throwOnTrivialRootCycle() { + val e = intercept[ConfigException.UnresolvedSubstitution] { + resolve(parseObject("""a = ${a}""")) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle")) + assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("${a}")) + } + + @Test + def throwOnSelfReferentialArrayInsideMergePiece() { + // an array with a substitution inside it is never self-referential, + // even when the field it is on was merged over a substitution + val e = intercept[ConfigException.UnresolvedSubstitution] { + resolve(parseObject("""d { x = [1] }, c : ${d}, c { x = [${c.x}, 2] }""")) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle")) + } + + @Test + def throwOnSelfReferentialObjectInsideMergePiece() { + val e = intercept[ConfigException.UnresolvedSubstitution] { + resolve(parseObject("""d { x = 1 }, c : ${d}, c { x = { i = ${c.x} } }""")) + } + assertTrue("Wrong exception: " + e.getMessage, e.getMessage.contains("cycle")) + } + @Test def ignoreHiddenUndefinedSubst() { // if a substitution is overridden then it shouldn't matter that it's undefined @@ -1613,4 +1671,16 @@ class ConfigSubstitutionTest extends TestUtils { assertNotSame(shared, cleared) assertSame(cleared, cleared.withFallbacksNotIgnored()) } + @Test + def reviewNestedSelfReferenceThroughMerge(): Unit = { + val resolved = resolve(parseObject("""d { x { y = [0] } }, c = ${d}, c { x { y = ${c.x.y}[1] } }""")) + assertEquals(Seq(0, 1), resolved.getIntList("c.x.y").asScala.toSeq) + } + + @Test + def reviewNestedOptionalSelfReferenceThroughMerge(): Unit = { + val resolved = resolve(parseObject("""d { x { y = [0] } }, c = ${d}, c { x { y = ${?c.x.y}[1] } }""")) + assertEquals(Seq(0, 1), resolved.getIntList("c.x.y").asScala.toSeq) + } + }