fix: Don't evaluate substitutions hidden by values from resolved objects (#839)

Author: johanandren

config/src/main/java/com/typesafe/config/impl/ConfigDelayedMerge.java

@@ -6,7 +6,9 @@
 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;
@@ -82,6 +84,16 @@ static ResolveResult<? extends AbstractConfigValue> resolveSubstitutions(Replace
         int count = 0;
         AbstractConfigValue merged = null;
         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
+            // ignores fallbacks, nothing below can contribute, so stop.
+            if (merged != null && merged.ignoresFallbacks()) {
+                if (ConfigImpl.traceSubstitutionsEnabled())
+                    ConfigImpl.trace(newContext.depth(),
+                            "merged ignores fallbacks, skipping remaining stack");
+                break;
+            }
+
             // the end value may or may not be resolved already
 
             ResolveSource sourceForEnd;
@@ -120,6 +132,23 @@ else if (end instanceof Unmergeable) {
                             "will resolve end against the original source with parent pushed");
 
                 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 (ConfigImpl.traceSubstitutionsEnabled()) {
@@ -152,6 +181,38 @@ 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<String, AbstractConfigValue> kept = new LinkedHashMap<String, AbstractConfigValue>();
+        boolean pruned = false;
+        for (String key : simple.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));
+            }
+        }
+        if (!pruned)
+            return end;
+        if (kept.isEmpty())
+            return null;
+        return new SimpleConfigObject(end.origin(), kept,
+                ResolveStatus.fromValues(kept.values()), false);
+    }
+
     @Override
     public AbstractConfigValue makeReplacement(ResolveContext context, int skipping) {
         return ConfigDelayedMerge.makeReplacement(context, stack, skipping);

config/src/test/scala/com/typesafe/config/impl/ConfigSubstitutionTest.scala

@@ -1277,4 +1277,70 @@ class ConfigSubstitutionTest extends TestUtils {
         val resolved2 = resolve(obj2)
         assertEquals(parseObject("{ x : 42, y : 42 }"), resolved2.getConfig("a").root)
     }
+
+    // Reproduces https://github.com/lightbend/config/issues/838
+    // A substitution hidden by a value that could not be merged with it should
+    // never be evaluated. This works when the hiding value is written directly,
+    // but not when it arrives via another substitution that resolves to an object.
+    @Test
+    def substitutionHiddenByObjectFromSubstitutionIsNotEvaluated() {
+        // Sanity: the direct-literal case works as the spec describes.
+        val direct = parseObject("""
+            p: { a : ${y} }
+            p: { a : 42 }
+            p: { a : { x : 1 } }
+        """)
+        val resolvedDirect = resolve(direct)
+        assertEquals(parseObject("""{ a : { x : 1 } }"""), resolvedDirect.getConfig("p").root)
+
+        // The middle hiding value is supplied via a substitution to an object.
+        // Per spec intent, ${y} should still never be evaluated.
+        val viaSub = parseObject("""
+            p: { a : ${y} }
+            p: ${x}
+            p: { a : { x : 1 } }
+            x: { a : 42 }
+        """)
+        val resolvedViaSub = resolve(viaSub)
+        assertEquals(parseObject("""{ a : { x : 1 } }"""), resolvedViaSub.getConfig("p").root)
+    }
+
+    // Spec (HOCON.md): "If a substitution is hidden by a value that could not
+    // be merged with it (by a non-object value) then it is never evaluated and
+    // no error will be reported." The direct top-level case from the spec.
+    @Test
+    def substitutionHiddenByLiteralNonObjectIsNotEvaluated() {
+        val obj = parseObject("""
+            foo: ${does-not-exist}
+            foo: 42
+        """)
+        val resolved = resolve(obj)
+        assertEquals(42, resolved.getInt("foo"))
+    }
+
+    // Variant of #838: an object containing a substitution is hidden by a
+    // literal non-object. The object cannot merge with 42, so the enclosed
+    // substitution must not be evaluated.
+    @Test
+    def substitutionInsideObjectHiddenByLiteralNonObjectIsNotEvaluated() {
+        val obj = parseObject("""
+            p: { a : ${does-not-exist} }
+            p: 42
+        """)
+        val resolved = resolve(obj)
+        assertEquals(42, resolved.getInt("p"))
+    }
+
+    // Variant of #838: an object containing a substitution is hidden by a
+    // substitution that resolves to a non-object. Same spec intent applies.
+    @Test
+    def substitutionInsideObjectHiddenByNonObjectFromSubstitutionIsNotEvaluated() {
+        val obj = parseObject("""
+            p: { a : ${does-not-exist} }
+            p: ${z}
+            z: 42
+        """)
+        val resolved = resolve(obj)
+        assertEquals(42, resolved.getInt("p"))
+    }
 }