Fix crash during destroy planning due to validation (#830)

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>

internal/checks/state_report.go

@@ -29,7 +29,8 @@ func (c *State) ReportCheckableObjects(configAddr addrs.ConfigCheckable, objectA
 	}
 	if st.objects.Elems != nil {
 		// Can only report checkable objects once per configuration object
-		panic(fmt.Sprintf("duplicate checkable objects report for %s ", configAddr))
+		// This is not a problem as the result is already cached.
+		return
 	}
 
 	// At this point we pre-populate all of the check results as StatusUnknown,

internal/tofu/graph_builder_eval.go

@@ -67,8 +67,8 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer {
 		},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
-		&ModuleVariableTransformer{Config: b.Config, Planning: true},
+		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
 			Config:   b.Config,

internal/tofu/graph_builder_plan.go

@@ -128,8 +128,8 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
-		&ModuleVariableTransformer{Config: b.Config, Planning: true},
+		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
 			Config:      b.Config,

internal/tofu/node_module_variable.go

@@ -25,10 +25,6 @@ type nodeExpandModuleVariable struct {
 	Module addrs.Module
 	Config *configs.Variable
 	Expr   hcl.Expression
-
-	// Planning must be set to true when building a planning graph, and must be
-	// false when building an apply graph.
-	Planning bool
 }
 
 var (
@@ -54,10 +50,8 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error
 	// We should only do this during planning as the apply phase starts with
 	// all the same checkable objects that were registered during the plan.
 	var checkableAddrs addrs.Set[addrs.Checkable]
-	if n.Planning {
+	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
-		if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
+		checkableAddrs = addrs.MakeSet[addrs.Checkable]()
-			checkableAddrs = addrs.MakeSet[addrs.Checkable]()
-		}
 	}
 
 	expander := ctx.InstanceExpander()

internal/tofu/node_module_variable_test.go

@@ -4,6 +4,7 @@
 package tofu
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 
@@ -13,7 +14,13 @@ import (
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/checks"
 	"github.com/opentofu/opentofu/internal/configs"
+	"github.com/opentofu/opentofu/internal/configs/configschema"
+	"github.com/opentofu/opentofu/internal/plans"
+	"github.com/opentofu/opentofu/internal/providers"
+	"github.com/opentofu/opentofu/internal/states"
+	"github.com/opentofu/opentofu/internal/tfdiags"
 )
 
 func TestNodeModuleVariablePath(t *testing.T) {
@@ -122,3 +129,178 @@ func TestNodeModuleVariableReference_grandchild(t *testing.T) {
 		t.Error(problem)
 	}
 }
+
+func TestNodeModuleVariableConstraints(t *testing.T) {
+	// This is a little extra convoluted to poke at some edge cases that have cropped up in the past around
+	// evaluating dependent nodes between the plan -> apply and destroy cycle.
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+			variable "input" {
+				type = string
+				validation {
+					condition = var.input != ""
+					error_message = "Input must not be empty."
+				}
+			}
+
+			module "child" {
+				source = "./child"
+				input = var.input
+			}
+
+			provider "test" {
+				alias = "secondary"
+				test_string = module.child.output
+			}
+
+			resource "test_object" "resource" {
+				provider = test.secondary
+				test_string = "test string"
+			}
+
+		`,
+		"child/main.tf": `
+			variable "input" {
+				type = string
+				validation {
+					condition = var.input != ""
+					error_message = "Input must not be empty."
+				}
+			}
+			provider "test" {
+				test_string = "foo"
+			}
+			resource "test_object" "resource" {
+				test_string = var.input
+			}
+			output "output" {
+				value = test_object.resource.id
+			}
+		`,
+	})
+
+	checkableObjects := []addrs.Checkable{
+		addrs.InputVariable{Name: "input"}.Absolute(addrs.RootModuleInstance),
+		addrs.InputVariable{Name: "input"}.Absolute(addrs.RootModuleInstance.Child("child", addrs.NoKey)),
+	}
+
+	p := &MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			Provider: providers.Schema{Block: simpleTestSchema()},
+			ResourceTypes: map[string]providers.Schema{
+				"test_object": providers.Schema{Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {
+							Type:     cty.String,
+							Computed: true,
+						},
+						"test_string": {
+							Type:     cty.String,
+							Required: true,
+						},
+					},
+				}},
+			},
+		},
+	}
+	p.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) {
+		if req.Config.GetAttr("test_string").IsNull() {
+			resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing test_string value"))
+		}
+		return resp
+	}
+
+	ctxOpts := &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	}
+
+	t.Run("pass", func(t *testing.T) {
+		ctx := testContext2(t, ctxOpts)
+		plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal("beep"),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		assertNoDiagnostics(t, diags)
+
+		for _, addr := range checkableObjects {
+			result := plan.Checks.GetObjectResult(addr)
+			if result == nil {
+				t.Fatalf("no check result for %s in the plan", addr)
+			}
+			if got, want := result.Status, checks.StatusPass; got != want {
+				t.Fatalf("wrong check status for %s during planning\ngot:  %s\nwant: %s", addr, got, want)
+			}
+		}
+
+		state, diags := ctx.Apply(plan, m)
+		assertNoDiagnostics(t, diags)
+		for _, addr := range checkableObjects {
+			result := state.CheckResults.GetObjectResult(addr)
+			if result == nil {
+				t.Fatalf("no check result for %s in the final state", addr)
+			}
+			if got, want := result.Status, checks.StatusPass; got != want {
+				t.Errorf("wrong check status for %s after apply\ngot:  %s\nwant: %s", addr, got, want)
+			}
+		}
+
+		plan, diags = ctx.Plan(m, state, &PlanOpts{
+			Mode: plans.DestroyMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal("beep"),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		assertNoDiagnostics(t, diags)
+
+		state, diags = ctx.Apply(plan, m)
+		assertNoDiagnostics(t, diags)
+		for _, addr := range checkableObjects {
+			result := state.CheckResults.GetObjectResult(addr)
+			if result == nil {
+				t.Fatalf("no check result for %s in the final state", addr)
+			}
+			if got, want := result.Status, checks.StatusPass; got != want {
+				t.Errorf("wrong check status for %s after apply\ngot:  %s\nwant: %s", addr, got, want)
+			}
+		}
+	})
+
+	t.Run("fail", func(t *testing.T) {
+		ctx := testContext2(t, ctxOpts)
+		_, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+			Mode: plans.NormalMode,
+			SetVariables: InputValues{
+				"input": &InputValue{
+					Value:      cty.StringVal(""),
+					SourceType: ValueFromCLIArg,
+				},
+			},
+		})
+		if !diags.HasErrors() {
+			t.Fatalf("succeeded; want error")
+		}
+
+		const wantSummary = "Invalid value for variable"
+		found := false
+		for _, diag := range diags {
+			if diag.Severity() == tfdiags.Error && diag.Description().Summary == wantSummary {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			t.Fatalf("missing expected error\nwant summary: %s\ngot: %s", wantSummary, diags.Err().Error())
+		}
+	})
+}

internal/tofu/node_root_variable.go

@@ -25,10 +25,6 @@ type NodeRootVariable struct {
 	// converted or validated, and can be nil for a variable that isn't
 	// set at all.
 	RawValue *InputValue
-
-	// Planning must be set to true when building a planning graph, and must be
-	// false when building an apply graph.
-	Planning bool
 }
 
 var (
@@ -87,12 +83,10 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di
 		}
 	}
 
-	if n.Planning {
+	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(addrs.RootModule)) {
-		if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(addrs.RootModule)) {
+		ctx.Checks().ReportCheckableObjects(
-			ctx.Checks().ReportCheckableObjects(
+			n.Addr.InModule(addrs.RootModule),
-				n.Addr.InModule(addrs.RootModule),
+			addrs.MakeSet[addrs.Checkable](n.Addr.Absolute(addrs.RootModuleInstance)))
-				addrs.MakeSet[addrs.Checkable](n.Addr.Absolute(addrs.RootModuleInstance)))
-		}
 	}
 
 	finalVal, moreDiags := prepareFinalInputVariableValue(

internal/tofu/node_root_variable_test.go

@@ -33,6 +33,14 @@ func TestNodeRootVariableExecute(t *testing.T) {
 			},
 		}
 
+		ctx.ChecksState = checks.NewState(&configs.Config{
+			Module: &configs.Module{
+				Variables: map[string]*configs.Variable{
+					"foo": n.Config,
+				},
+			},
+		})
+
 		diags := n.Execute(ctx, walkApply)
 		if diags.HasErrors() {
 			t.Fatalf("unexpected error: %s", diags.Err())
@@ -116,7 +124,6 @@ func TestNodeRootVariableExecute(t *testing.T) {
 				Value:      cty.StringVal("5"),
 				SourceType: ValueFromUnknown,
 			},
-			Planning: true,
 		}
 
 		ctx.ChecksState = checks.NewState(&configs.Config{

internal/tofu/transform_module_variable.go

@@ -28,10 +28,6 @@ import (
 // steps for validating module blocks, separate from this transform.
 type ModuleVariableTransformer struct {
 	Config *configs.Config
-
-	// Planning must be set to true when building a planning graph, and must be
-	// false when building an apply graph.
-	Planning bool
 }
 
 func (t *ModuleVariableTransformer) Transform(g *Graph) error {
@@ -110,10 +106,9 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, c *configs
 			Addr: addrs.InputVariable{
 				Name: v.Name,
 			},
-			Module:   c.Path,
+			Module: c.Path,
-			Config:   v,
+			Config: v,
-			Expr:     expr,
+			Expr:   expr,
-			Planning: t.Planning,
 		}
 		g.Add(node)
 	}

internal/tofu/transform_variable.go

@@ -18,10 +18,6 @@ type RootVariableTransformer struct {
 	Config *configs.Config
 
 	RawValues InputValues
-
-	// Planning must be set to true when building a planning graph, and must be
-	// false when building an apply graph.
-	Planning bool
 }
 
 func (t *RootVariableTransformer) Transform(g *Graph) error {
@@ -42,7 +38,6 @@ func (t *RootVariableTransformer) Transform(g *Graph) error {
 			},
 			Config:   v,
 			RawValue: t.RawValues[v.Name],
-			Planning: t.Planning,
 		}
 		g.Add(node)
 	}