Merge pull request #305 from hashicorp/bendbennett/issues-303

Fix Breaking change of null keeper values between 3.3.2 and 3.4.0

.github/workflows/test.yml

@@ -50,7 +50,7 @@ jobs:
     name: 'Acc. Tests (OS: ${{ matrix.os }} / TF: ${{ matrix.terraform }})'
     needs: build
     runs-on: ${{ matrix.os }}
-    timeout-minutes: 15
+    timeout-minutes: 20
     strategy:
       fail-fast: false
       matrix:

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 BUG FIXES:
 
+* all: Prevent `keeper` with `null` values from forcing replacement ([305](https://github.com/hashicorp/terraform-provider-random/pull/305)).
 * resource/random_password: During upgrade state, ensure `min_upper` is populated ([304](https://github.com/hashicorp/terraform-provider-random/pull/304)).
 * resource/random_string: During upgrade state, ensure `min_upper` is populated ([304](https://github.com/hashicorp/terraform-provider-random/pull/304)).
 

internal/planmodifiers/attribute.go

@@ -143,3 +143,121 @@ func (d *numberNumericAttributePlanModifier) Modify(ctx context.Context, req tfs
 		return
 	}
 }
+
+func RequiresReplaceIfValuesNotNull() tfsdk.AttributePlanModifier {
+	return requiresReplaceIfValuesNotNullModifier{}
+}
+
+type requiresReplaceIfValuesNotNullModifier struct{}
+
+func (r requiresReplaceIfValuesNotNullModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, resp *tfsdk.ModifyAttributePlanResponse) {
+	if req.AttributeConfig == nil || req.AttributePlan == nil || req.AttributeState == nil {
+		// shouldn't happen, but let's not panic if it does
+		return
+	}
+
+	if req.State.Raw.IsNull() {
+		// if we're creating the resource, no need to delete and
+		// recreate it
+		return
+	}
+
+	if req.Plan.Raw.IsNull() {
+		// if we're deleting the resource, no need to delete and
+		// recreate it
+		return
+	}
+
+	// If there are no differences, do not mark the resource for replacement
+	// and ensure the plan matches the configuration.
+	if req.AttributeConfig.Equal(req.AttributeState) {
+		return
+	}
+
+	if req.AttributeState.IsNull() {
+		// terraform-plugin-sdk would store maps as null if all keys had null
+		// values. To prevent unintentional replacement plans when migrating
+		// to terraform-plugin-framework, only trigger replacement when the
+		// prior state (map) is null and when there are not null map values.
+		allNullValues := true
+
+		configMap, ok := req.AttributeConfig.(types.Map)
+
+		if !ok {
+			return
+		}
+
+		for _, configValue := range configMap.Elems {
+			if !configValue.IsNull() {
+				allNullValues = false
+			}
+		}
+
+		if allNullValues {
+			return
+		}
+	} else {
+		// terraform-plugin-sdk would completely omit storing map keys with
+		// null values, so this also must prevent unintentional replacement
+		// in that case as well.
+		allNewNullValues := true
+
+		configMap, ok := req.AttributeConfig.(types.Map)
+
+		if !ok {
+			return
+		}
+
+		stateMap, ok := req.AttributeState.(types.Map)
+
+		if !ok {
+			return
+		}
+
+		for configKey, configValue := range configMap.Elems {
+			stateValue, ok := stateMap.Elems[configKey]
+
+			// If the key doesn't exist in state and the config value is
+			// null, do not trigger replacement.
+			if !ok && configValue.IsNull() {
+				continue
+			}
+
+			// If the state value exists and it is equal to the config value,
+			// do not trigger replacement.
+			if configValue.Equal(stateValue) {
+				continue
+			}
+
+			allNewNullValues = false
+			break
+		}
+
+		for stateKey := range stateMap.Elems {
+			_, ok := configMap.Elems[stateKey]
+
+			// If the key doesn't exist in the config, but there is a state
+			// value, trigger replacement.
+			if !ok {
+				allNewNullValues = false
+				break
+			}
+		}
+
+		if allNewNullValues {
+			return
+		}
+	}
+
+	resp.RequiresReplace = true
+}
+
+// Description returns a human-readable description of the plan modifier.
+func (r requiresReplaceIfValuesNotNullModifier) Description(ctx context.Context) string {
+	return "If the value of this attribute changes, Terraform will destroy and recreate the resource."
+}
+
+// MarkdownDescription returns a markdown description of the plan modifier.
+func (r requiresReplaceIfValuesNotNullModifier) MarkdownDescription(ctx context.Context) string {
+	return "If the value of this attribute changes, Terraform will destroy and recreate the resource."
+}

internal/provider/resource_id.go

@@ -16,6 +16,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/types"
 
 	"github.com/terraform-providers/terraform-provider-random/internal/diagnostics"
+	"github.com/terraform-providers/terraform-provider-random/internal/planmodifiers"
 )
 
 var _ provider.ResourceType = (*idResourceType)(nil)
@@ -47,7 +48,7 @@ exist concurrently.
 				},
 				Optional: true,
 				PlanModifiers: []tfsdk.AttributePlanModifier{
-					resource.RequiresReplace(),
+					planmodifiers.RequiresReplaceIfValuesNotNull(),
 				},
 			},
 			"byte_length": {
@@ -73,27 +74,42 @@ exist concurrently.
 					"case-sensitive letters, digits and the characters `_` and `-`.",
 				Type:     types.StringType,
 				Computed: true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					resource.UseStateForUnknown(),
+				},
 			},
 			"b64_std": {
 				Description: "The generated id presented in base64 without additional transformations.",
 				Type:        types.StringType,
 				Computed:    true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					resource.UseStateForUnknown(),
+				},
 			},
 			"hex": {
 				Description: "The generated id presented in padded hexadecimal digits. This result will " +
 					"always be twice as long as the requested byte length.",
 				Type:     types.StringType,
 				Computed: true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					resource.UseStateForUnknown(),
+				},
 			},
 			"dec": {
 				Description: "The generated id presented in non-padded decimal digits.",
 				Type:        types.StringType,
 				Computed:    true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					resource.UseStateForUnknown(),
+				},
 			},
 			"id": {
 				Description: "The generated id presented in base64 without additional transformations or prefix.",
 				Type:        types.StringType,
 				Computed:    true,
+				PlanModifiers: []tfsdk.AttributePlanModifier{
+					resource.UseStateForUnknown(),
+				},
 			},
 		},
 	}, nil
@@ -163,9 +179,17 @@ func (r *idResource) Create(ctx context.Context, req resource.CreateRequest, res
 func (r *idResource) Read(context.Context, resource.ReadRequest, *resource.ReadResponse) {
 }
 
-// Update is intentionally left blank as all required and optional attributes force replacement of the resource
-// through the RequiresReplace AttributePlanModifier.
-func (r *idResource) Update(context.Context, resource.UpdateRequest, *resource.UpdateResponse) {
+// Update ensures the plan value is copied to the state to complete the update.
+func (r *idResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
+	var model idModelV0
+
+	resp.Diagnostics.Append(req.Plan.Get(ctx, &model)...)
+
+	if resp.Diagnostics.HasError() {
+		return
+	}
+
+	resp.Diagnostics.Append(resp.State.Set(ctx, &model)...)
 }
 
 // Delete does not need to explicitly call resp.State.RemoveResource() as this is automatically handled by the