fix(planmodifier): requires replace external values should not replace if state value is null (#866)

* fix(planmodifier): requires replace external values should not replace if wither plan or state is null

fixes importing resource with replace_triggers_external_values recreates the resource #858

* test(planmodifier): add comments explaining why dummy plan and state are used

* refactor(planmodifier): simplify RequiredReplaceIfNotNull to avoid duplicate equality checks

* docs(planmodifier): clarify code comment

Author: matt-FFFFFF

internal/services/myplanmodifier/planmodifierdynamic/dynamic_requires_replace.go

@@ -21,17 +21,22 @@ func RequiresReplace() planmodifier.Dynamic {
 	)
 }
 
+// RequiresReplaceIfNotNull is a plan modifier that sets RequiresReplace
+// on the attribute if the planned value is different from the state value.
+// It will not trigger replacement when either the planned or state value is null.
+// When this function is called, the equality of the planned and state values
+// has already been checked by the PlanModifyDynamic method, so we can assume they are different values.
 func RequiresReplaceIfNotNull() planmodifier.Dynamic {
 	return RequiresReplaceIf(
 		func(_ context.Context, req planmodifier.DynamicRequest, resp *RequiresReplaceIfFuncResponse) {
-			if req.PlanValue.IsNull() {
+			if req.PlanValue.IsNull() || req.StateValue.IsNull() {
 				resp.RequiresReplace = false
 				return
 			}
 			resp.RequiresReplace = true
 		},
-		"If the value of this attribute changes and is not null, Terraform will destroy and recreate the resource.",
-		"If the value of this attribute changes and is not null, Terraform will destroy and recreate the resource.",
+		"If the planned value is different from the state value, Terraform will destroy and recreate the resource unless either the planned or state value is null.",
+		"If the planned value is different from the state value, Terraform will destroy and recreate the resource unless either the planned or state value is null.",
 	)
 }
 

internal/services/myplanmodifier/planmodifierdynamic/dynamic_requires_replace_test.go

@@ -0,0 +1,96 @@
+package planmodifierdynamic
+
+import (
+	"context"
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
+	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
+	"github.com/hashicorp/terraform-plugin-framework/types"
+	"github.com/hashicorp/terraform-plugin-go/tftypes"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_RequiresReplaceIfNotNull(t *testing.T) {
+	tests := []struct {
+		name           string
+		planValue      types.Dynamic
+		stateValue     types.Dynamic
+		expectedResult bool
+	}{
+		{
+			name:           "both values null",
+			planValue:      types.DynamicNull(),
+			stateValue:     types.DynamicNull(),
+			expectedResult: false,
+		},
+		{
+			name:           "plan value null",
+			planValue:      types.DynamicNull(),
+			stateValue:     types.DynamicValue(types.StringValue("state")),
+			expectedResult: false,
+		},
+		{
+			name:           "state value null",
+			planValue:      types.DynamicValue(types.StringValue("plan")),
+			stateValue:     types.DynamicNull(),
+			expectedResult: false,
+		},
+		{
+			name:           "values equal",
+			planValue:      types.DynamicValue(types.StringValue("same")),
+			stateValue:     types.DynamicValue(types.StringValue("same")),
+			expectedResult: false,
+		},
+		{
+			name:           "values different",
+			planValue:      types.DynamicValue(types.StringValue("plan")),
+			stateValue:     types.DynamicValue(types.StringValue("state")),
+			expectedResult: true,
+		},
+	}
+
+	// We need to create a dummy state and plan because the PlanModifierDynamic
+	// method checks for a null state and plan, returning early if they are null.
+	state := tftypes.NewValue(tftypes.Object{
+		AttributeTypes: map[string]tftypes.Type{
+			"test": tftypes.String,
+		},
+	}, map[string]tftypes.Value{
+		"test": tftypes.NewValue(tftypes.String, "state"),
+	})
+
+	plan := tftypes.NewValue(tftypes.Object{
+		AttributeTypes: map[string]tftypes.Type{
+			"test": tftypes.String,
+		},
+	}, map[string]tftypes.Value{
+		"test": tftypes.NewValue(tftypes.String, "plan"),
+	})
+
+	// Set up the plan and state values for the test cases
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			modifier := RequiresReplaceIfNotNull()
+
+			req := planmodifier.DynamicRequest{
+				State: tfsdk.State{
+					Raw: state,
+				},
+				Plan: tfsdk.Plan{
+					Raw: plan,
+				},
+				PlanValue:  tt.planValue,
+				StateValue: tt.stateValue,
+			}
+
+			resp := &planmodifier.DynamicResponse{}
+
+			modifier.PlanModifyDynamic(context.Background(), req, resp)
+
+			require.Empty(t, resp.Diagnostics)
+			assert.Equal(t, tt.expectedResult, resp.RequiresReplace)
+		})
+	}
+}