From 9675c2f748a51d81047db3da61c767e4cf53f53a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Andr=C3=A9n?= Date: Mon, 4 May 2026 13:34:01 +0200 Subject: [PATCH] fix: regression - partially-shadowed object has nested delayed merge --- .../config/impl/ConfigDelayedMerge.java | 63 +++++++------------ .../config/impl/ConfigSubstitutionTest.scala | 33 ++++++++++ 2 files changed, 56 insertions(+), 40 deletions(-) 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 df29840f..68fd3997 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java @@ -6,9 +6,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import com.typesafe.config.ConfigException; import com.typesafe.config.ConfigOrigin; @@ -134,20 +132,19 @@ else if (end instanceof Unmergeable) { sourceForEnd = source.pushParent(replaceable); // Same spec rule as the short-circuit above, applied per-key: - // keys in the lower-priority 'end' object that are already - // shadowed in 'merged' by a value ignoring fallbacks would be - // discarded by the subsequent merge, so don't resolve them. - if (merged instanceof AbstractConfigObject && end instanceof AbstractConfigObject) { - AbstractConfigObject prunedEnd = pruneShadowedKeys( - (AbstractConfigObject) end, (AbstractConfigObject) merged); - if (prunedEnd == null) { - if (ConfigImpl.traceSubstitutionsEnabled()) - ConfigImpl.trace(newContext.depth(), - "all keys in end are shadowed by merged, skipping"); - count += 1; - continue; - } - end = prunedEnd; + // if every key in the lower-priority 'end' object is already + // shadowed in 'merged' by a value ignoring fallbacks, the whole + // 'end' would be discarded by the subsequent merge. Skip + // resolving it. (We only skip the whole entry — substituting a + // partial copy of 'end' would change identity and break parent + // chain walks during inner substitution resolution.) + if (merged instanceof AbstractConfigObject && end instanceof SimpleConfigObject + && allKeysShadowed((SimpleConfigObject) end, (AbstractConfigObject) merged)) { + if (ConfigImpl.traceSubstitutionsEnabled()) + ConfigImpl.trace(newContext.depth(), + "all keys in end are shadowed by merged, skipping"); + count += 1; + continue; } } @@ -181,36 +178,22 @@ else if (end instanceof Unmergeable) { return ResolveResult.make(newContext, merged); } - // Returns a copy of 'end' with keys removed when 'merged' already has a - // value at that key which ignores fallbacks. Returns null if every key in - // 'end' is shadowed. Returns 'end' unchanged if no pruning applies or if - // we cannot safely inspect merged (e.g. unresolved CDMO). - private static AbstractConfigObject pruneShadowedKeys(AbstractConfigObject end, - AbstractConfigObject merged) { - if (!(end instanceof SimpleConfigObject)) - return end; - SimpleConfigObject simple = (SimpleConfigObject) end; - Map kept = new LinkedHashMap(); - boolean pruned = false; - for (String key : simple.keySet()) { + // True when every key in 'end' is shadowed by a value in 'merged' that + // ignores fallbacks (so the subsequent merge would drop the whole 'end'). + private static boolean allKeysShadowed(SimpleConfigObject end, AbstractConfigObject merged) { + if (end.isEmpty()) + return false; + for (String key : end.keySet()) { AbstractConfigValue mergedValue; try { mergedValue = merged.attemptPeekWithPartialResolve(key); } catch (ConfigException.NotResolved e) { - return end; - } - if (mergedValue != null && mergedValue.ignoresFallbacks()) { - pruned = true; - } else { - kept.put(key, simple.attemptPeekWithPartialResolve(key)); + return false; } + if (mergedValue == null || !mergedValue.ignoresFallbacks()) + return false; } - if (!pruned) - return end; - if (kept.isEmpty()) - return null; - return new SimpleConfigObject(end.origin(), kept, - ResolveStatus.fromValues(kept.values()), false); + return true; } @Override 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 7a3953e4..b077e406 100644 --- a/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala @@ -1343,4 +1343,37 @@ class ConfigSubstitutionTest extends TestUtils { val resolved = resolve(obj) assertEquals(42, resolved.getInt("p")) } + + // Regression: when a delayed-merge object's stack contains a SimpleConfigObject + // whose keys are partially shadowed by a higher-priority entry, pruning + // produces a SimpleConfigObject containing only the kept keys. If a kept key + // holds a nested ConfigDelayedMerge with an unmergeable element, resolving + // that inner CDM walks up the parent chain and reaches the outer CDMO, which + // still has the original (unpruned) entry in its stack — leading to + // ConfigException.BugOrBroken("tried to replace ... which is not in [...]"). + @Test + def partiallyShadowedObjectWithInnerDelayedMergeResolves() { + // ${m} between the two `p:` definitions forces `p` into a + // ConfigDelayedMergeObject (the merge cannot happen at parse time). + // The lower-priority entry (defined first) has keys a, b, c. The + // higher-priority entry (defined last) shadows a and b but does not + // define c, so pruning would yield {c: ...}. The kept c is itself a + // ConfigDelayedMerge {x: [${?u}, "default"]} containing an optional + // substitution, whose resolution walks back through the path to the + // outer CDMO. + val obj = parseObject(""" + p: { + a: "low" + b: "low" + c: { x: "default", x: ${?u} } + } + p: ${m} + p: { a: "high", b: "high" } + m: {} + """) + val resolved = resolve(obj) + assertEquals("high", resolved.getString("p.a")) + assertEquals("high", resolved.getString("p.b")) + assertEquals("default", resolved.getString("p.c.x")) + } }