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

@@ -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.

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())
+}