From 5f59572cb300b222dbd3aef8f414de8032ff4936 Mon Sep 17 00:00:00 2001
From: Pieter Noordhuis <pieter.noordhuis@databricks.com>
Date: Fri, 16 Feb 2024 17:19:40 +0100
Subject: [PATCH] Fix issue where interpolating a new ref would rewrite
 unrelated fields (#1217)

## Changes

When resolving a value returned by the lookup function, the code would
call into `resolveRef` with the key that `resolveKey` was called with.
In doing so, it would cache the _new_ ref under that key.

We fix this by caching ref resolution only at the top level and relying
on lookup caching to avoid duplicate work.

This came up while testing #1098.

## Tests

Unit test.
---
 libs/dyn/dynvar/resolve.go      | 18 +++++----------
 libs/dyn/dynvar/resolve_test.go | 40 +++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/libs/dyn/dynvar/resolve.go b/libs/dyn/dynvar/resolve.go
index b5417cac..b8a0aef6 100644
--- a/libs/dyn/dynvar/resolve.go
+++ b/libs/dyn/dynvar/resolve.go
@@ -105,21 +105,17 @@ func (r *resolver) resolveVariableReferences() (err error) {
 	keys := maps.Keys(r.refs)
 	sort.Strings(keys)
 	for _, key := range keys {
-		_, err := r.resolveRef(key, r.refs[key], []string{key})
+		v, err := r.resolveRef(r.refs[key], []string{key})
 		if err != nil {
 			return err
 		}
+		r.resolved[key] = v
 	}
 
 	return nil
 }
 
-func (r *resolver) resolveRef(key string, ref ref, seen []string) (dyn.Value, error) {
-	// Check if we have already resolved this variable reference.
-	if v, ok := r.resolved[key]; ok {
-		return v, nil
-	}
-
+func (r *resolver) resolveRef(ref ref, seen []string) (dyn.Value, error) {
 	// This is an unresolved variable reference.
 	deps := ref.references()
 
@@ -154,7 +150,6 @@ func (r *resolver) resolveRef(key string, ref ref, seen []string) (dyn.Value, er
 	if ref.isPure() && complete {
 		// If the variable reference is pure, we can substitute it.
 		// This is useful for interpolating values of non-string types.
-		r.resolved[key] = resolved[0]
 		return resolved[0], nil
 	}
 
@@ -178,10 +173,7 @@ func (r *resolver) resolveRef(key string, ref ref, seen []string) (dyn.Value, er
 		ref.str = strings.Replace(ref.str, ref.matches[j][0], s, 1)
 	}
 
-	// Store the interpolated value.
-	v := dyn.NewValue(ref.str, ref.value.Location())
-	r.resolved[key] = v
-	return v, nil
+	return dyn.NewValue(ref.str, ref.value.Location()), nil
 }
 
 func (r *resolver) resolveKey(key string, seen []string) (dyn.Value, error) {
@@ -211,7 +203,7 @@ func (r *resolver) resolveKey(key string, seen []string) (dyn.Value, error) {
 	// If the returned value is a valid variable reference, resolve it.
 	ref, ok := newRef(v)
 	if ok {
-		v, err = r.resolveRef(key, ref, seen)
+		v, err = r.resolveRef(ref, seen)
 	}
 
 	// Cache the return value and return to the caller.
diff --git a/libs/dyn/dynvar/resolve_test.go b/libs/dyn/dynvar/resolve_test.go
index 1234b7cb..304ed939 100644
--- a/libs/dyn/dynvar/resolve_test.go
+++ b/libs/dyn/dynvar/resolve_test.go
@@ -207,3 +207,43 @@ func TestResolveWithSkipEverything(t *testing.T) {
 	assert.Equal(t, "${b} ${a} ${a} ${b}", getByPath(t, out, "f").MustString())
 	assert.Equal(t, "${d} ${c} ${c} ${d}", getByPath(t, out, "g").MustString())
 }
+
+func TestResolveWithInterpolateNewRef(t *testing.T) {
+	in := dyn.V(map[string]dyn.Value{
+		"a": dyn.V("a"),
+		"b": dyn.V("${a}"),
+	})
+
+	// The call replaces ${a} with ${foobar} and skips everything else.
+	out, err := dynvar.Resolve(in, func(path dyn.Path) (dyn.Value, error) {
+		if path.String() == "a" {
+			return dyn.V("${foobar}"), nil
+		}
+		return dyn.InvalidValue, dynvar.ErrSkipResolution
+	})
+
+	require.NoError(t, err)
+	assert.Equal(t, "a", getByPath(t, out, "a").MustString())
+	assert.Equal(t, "${foobar}", getByPath(t, out, "b").MustString())
+}
+
+func TestResolveWithInterpolateAliasedRef(t *testing.T) {
+	in := dyn.V(map[string]dyn.Value{
+		"a": dyn.V("a"),
+		"b": dyn.V("${a}"),
+		"c": dyn.V("${x}"),
+	})
+
+	// The call replaces ${x} with ${b} and skips everything else.
+	out, err := dynvar.Resolve(in, func(path dyn.Path) (dyn.Value, error) {
+		if path.String() == "x" {
+			return dyn.V("${b}"), nil
+		}
+		return dyn.GetByPath(in, path)
+	})
+
+	require.NoError(t, err)
+	assert.Equal(t, "a", getByPath(t, out, "a").MustString())
+	assert.Equal(t, "a", getByPath(t, out, "b").MustString())
+	assert.Equal(t, "a", getByPath(t, out, "c").MustString())
+}