Resolve variables in a loop (#2164)

denik · web-flow · commit 2e70558dc18f · 2025-01-16T14:39:54.000Z
## Changes - Instead of doing 2 passes on variable resolution, do a loop until there are no more updates (or we reach count 100). - Stacked on top of #2163 which is a regression test for this: acceptance/bundle/variables/complex-transitive-deep ## Tests Existing tests, new regression tests. These tests already passed before, added for completeness: - acceptance/bundle/variables/cycle - acceptance/bundle/variables/complex-cross-ref
diff --git a/acceptance/bundle/variables/complex-cross-ref/databricks.yml b/acceptance/bundle/variables/complex-cross-ref/databricks.yml
@@ -0,0 +1,12 @@
+bundle:
+  name: complex-cross-ref
+
+variables:
+  a:
+    default:
+      a_1: 500
+      a_2: ${var.b.b_2}
+  b:
+    default:
+      b_1: ${var.a.a_1}
+      b_2: 2.5
diff --git a/acceptance/bundle/variables/complex-cross-ref/output.txt b/acceptance/bundle/variables/complex-cross-ref/output.txt
@@ -0,0 +1,22 @@
+{
+  "a": {
+    "default": {
+      "a_1": 500,
+      "a_2": 2.5
+    },
+    "value": {
+      "a_1": 500,
+      "a_2": 2.5
+    }
+  },
+  "b": {
+    "default": {
+      "b_1": 500,
+      "b_2": 2.5
+    },
+    "value": {
+      "b_1": 500,
+      "b_2": 2.5
+    }
+  }
+}
diff --git a/acceptance/bundle/variables/complex-cross-ref/script b/acceptance/bundle/variables/complex-cross-ref/script
@@ -0,0 +1 @@
+$CLI bundle validate -o json | jq .variables
diff --git a/acceptance/bundle/variables/complex-cycle-self/databricks.yml b/acceptance/bundle/variables/complex-cycle-self/databricks.yml
@@ -0,0 +1,7 @@
+bundle:
+  name: cycle
+
+variables:
+  a:
+    default:
+      hello: ${var.a}
diff --git a/acceptance/bundle/variables/complex-cycle-self/output.txt b/acceptance/bundle/variables/complex-cycle-self/output.txt
@@ -0,0 +1,9 @@
+Warning: Detected unresolved variables after 11 resolution rounds
+
+Name: cycle
+Target: default
+Workspace:
+  User: $USERNAME
+  Path: /Workspace/Users/$USERNAME/.bundle/cycle/default
+
+Found 1 warning
diff --git a/acceptance/bundle/variables/complex-cycle-self/script b/acceptance/bundle/variables/complex-cycle-self/script
@@ -0,0 +1 @@
+$CLI bundle validate
diff --git a/acceptance/bundle/variables/complex-cycle/databricks.yml b/acceptance/bundle/variables/complex-cycle/databricks.yml
@@ -0,0 +1,10 @@
+bundle:
+  name: cycle
+
+variables:
+  a:
+    default:
+      hello: ${var.b}
+  b:
+    default:
+      hello: ${var.a}
diff --git a/acceptance/bundle/variables/complex-cycle/output.txt b/acceptance/bundle/variables/complex-cycle/output.txt
@@ -0,0 +1,9 @@
+Warning: Detected unresolved variables after 11 resolution rounds
+
+Name: cycle
+Target: default
+Workspace:
+  User: $USERNAME
+  Path: /Workspace/Users/$USERNAME/.bundle/cycle/default
+
+Found 1 warning
diff --git a/acceptance/bundle/variables/complex-cycle/script b/acceptance/bundle/variables/complex-cycle/script
@@ -0,0 +1 @@
+$CLI bundle validate
diff --git a/acceptance/bundle/variables/complex-transitive-deep/output.txt b/acceptance/bundle/variables/complex-transitive-deep/output.txt
@@ -1,3 +1,3 @@
 {
-  "spark.databricks.sql.initial.catalog.name": "${var.catalog}"
+  "spark.databricks.sql.initial.catalog.name": "hive_metastore"
 }
diff --git a/acceptance/bundle/variables/cycle/databricks.yml b/acceptance/bundle/variables/cycle/databricks.yml
@@ -0,0 +1,8 @@
+bundle:
+  name: cycle
+
+variables:
+  a:
+    default: ${var.b}
+  b:
+    default: ${var.a}
diff --git a/acceptance/bundle/variables/cycle/output.txt b/acceptance/bundle/variables/cycle/output.txt
@@ -0,0 +1,14 @@
+Error: cycle detected in field resolution: variables.a.default -> var.b -> var.a -> var.b
+
+{
+  "a": {
+    "default": "${var.b}",
+    "value": "${var.b}"
+  },
+  "b": {
+    "default": "${var.a}",
+    "value": "${var.a}"
+  }
+}
+
+Exit code: 1
diff --git a/acceptance/bundle/variables/cycle/script b/acceptance/bundle/variables/cycle/script
@@ -0,0 +1 @@
+$CLI bundle validate -o json | jq .variables
diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go
@@ -3,6 +3,7 @@ package mutator
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config"
@@ -13,15 +14,37 @@ import (
 	"github.com/databricks/cli/libs/dyn/dynvar"
 )
 
+/*
+For pathological cases, output and time grow exponentially.
+
+On my laptop, timings for acceptance/bundle/variables/complex-cycle:
+rounds           time
+
+	 9          0.10s
+	10          0.13s
+	11          0.27s
+	12          0.68s
+	13          1.98s
+	14          6.28s
+	15         21.70s
+	16         78.16s
+*/
+const maxResolutionRounds = 11
+
 type resolveVariableReferences struct {
-	prefixes []string
-	pattern  dyn.Pattern
-	lookupFn func(dyn.Value, dyn.Path, *bundle.Bundle) (dyn.Value, error)
-	skipFn   func(dyn.Value) bool
+	prefixes    []string
+	pattern     dyn.Pattern
+	lookupFn    func(dyn.Value, dyn.Path, *bundle.Bundle) (dyn.Value, error)
+	skipFn      func(dyn.Value) bool
+	extraRounds int
 }
 
 func ResolveVariableReferences(prefixes ...string) bundle.Mutator {
-	return &resolveVariableReferences{prefixes: prefixes, lookupFn: lookup}
+	return &resolveVariableReferences{
+		prefixes:    prefixes,
+		lookupFn:    lookup,
+		extraRounds: maxResolutionRounds - 1,
+	}
 }
 
 func ResolveVariableReferencesInLookup() bundle.Mutator {
@@ -86,7 +109,36 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
 	varPath := dyn.NewPath(dyn.Key("var"))
 
 	var diags diag.Diagnostics
+	maxRounds := 1 + m.extraRounds
+
+	for round := range maxRounds {
+		hasUpdates, newDiags := m.resolveOnce(b, prefixes, varPath)
+
+		diags = diags.Extend(newDiags)
+
+		if diags.HasError() {
+			break
+		}
+
+		if !hasUpdates {
+			break
+		}
 
+		if round >= maxRounds-1 {
+			diags = diags.Append(diag.Diagnostic{
+				Severity: diag.Warning,
+				Summary:  fmt.Sprintf("Detected unresolved variables after %d resolution rounds", round+1),
+				// Would be nice to include names of the variables there, but that would complicate things more
+			})
+			break
+		}
+	}
+	return diags
+}
+
+func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn.Path, varPath dyn.Path) (bool, diag.Diagnostics) {
+	var diags diag.Diagnostics
+	hasUpdates := false
 	err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) {
 		// Synthesize a copy of the root that has all fields that are present in the type
 		// but not set in the dynamic value set to their corresponding empty value.
@@ -129,6 +181,7 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
 						if m.skipFn != nil && m.skipFn(v) {
 							return dyn.InvalidValue, dynvar.ErrSkipResolution
 						}
+						hasUpdates = true
 						return m.lookupFn(normalized, path, b)
 					}
 				}
@@ -149,5 +202,6 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
 	if err != nil {
 		diags = diags.Extend(diag.FromErr(err))
 	}
-	return diags
+
+	return hasUpdates, diags
 }
diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go
@@ -66,11 +66,6 @@ func Initialize() bundle.Mutator {
 				"workspace",
 				"variables",
 			),
-			mutator.ResolveVariableReferences(
-				"bundle",
-				"workspace",
-				"variables",
-			),
 
 			mutator.MergeJobClusters(),
 			mutator.MergeJobParameters(),

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+$CLI bundle validate -o json \| jq .variables`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,3 @@`
`1`	`1`	`{`
`2`		`- "spark.databricks.sql.initial.catalog.name": "${var.catalog}"`
	`2`	`+ "spark.databricks.sql.initial.catalog.name": "hive_metastore"`
`3`	`3`	`}`