Strange behavior with Computed attributes after v0.4.2 upgrade

[ewbankkit]

I upgraded to terraform-plugin-framework@v0.4.2 and am now seeing some strange behavior.
A resource has an attribute

"function_name": {
	Description: "The name of the Lambda function, up to 64 characters in length. If you don't specify a name, AWS CloudFormation generates one.",
	Type:        types.StringType,
	Optional:    true,
	Computed:    true,
	Validators: []tfsdk.AttributeValidator{
		validate.StringLenAtLeast(1),
	},
	PlanModifiers: []tfsdk.AttributePlanModifier{
		tfsdk.RequiresReplace(),
	},
},

When terraform plan shows a difference in another attribute (description in this case which is Optional and is now getting a value):

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # cloud_service_thing.test must be replaced
-/+ resource "cloud_service_thing" "test" {
      ~ arn                 = "..." -> (known after apply)
      + description         = "test"
      ~ function_name       = "1WlhknThPCtZOiZjwt283Olmq-0A8jrKEtMBpJ" -> (known after apply) # forces replacement
      ~ id                  = "1WlhknThPCtZOiZjwt283Olmq-0A8jrKEtMBpJ" -> (known after apply)
        # (9 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

The attribute function_name (which hasn't changed) is showing a change (and a replacement as it's ForceNew).
arn and id are also both Computed attributes and their values have not changed.
This behavior was not observed with v0.4.0.

[bflad]

This behavior is (unfortunately) expected with the current implementation of RequiresReplace(), which will unilaterally mark an attribute for replacement any time there is any plan difference, even outside the scope of the current attribute.

As shown in hashicorp/terraform-provider-awscc#222 this behavior can be improved with the following workaround:

tfsdk.RequiresReplaceIf(func(ctx context.Context, state, config attr.Value, path *tftypes.AttributePath) (bool, diag.Diagnostics) {
	// TODO: Use config.IsNull() when available
	// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/193
	if config.Equal(state) || config.Equal(types.String{Null: true}) {
		return false, nil
	}

	return true, nil
}, "require replacement if configuration value changes", "require replacement if configuration value changes"),

Whether either or both parts of the conditional there should be part of the RequiresReplace() implementation is an interesting design consideration. I would posit that most folks would expect it to work similarly to ForceNew in the older Terraform Plugin SDK, which only marked the path for replacement when configuration values changed.

[paddycarver]

So the solution we came up with to fix this is to:

1. only set RequiresReplace to true if the value for that attribute, specifically, is actually changing. This seems reasonable to me.
2. only set RequiresReplace to true if the config is not null.

As I'm writing tests and leaving a comment for why that exception is carved out, I'm struggling here. Why did we decide that was important? Is it only because we're trying to control for the scenario where Optional+Computed attributes aren't set, and the provider changes them, in which case the practitioner likely doesn't expect the resource to be destroyed and recreated? If so, should that conditional only apply to computed attributes?

[bflad]

Indeed, the second rule:

only set RequiresReplace to true if the config is not null.

Should probably only apply to Computed attributes. For strictly Optional attributes, removing the configuration of an attribute should likely be seen as a change that requires replacement.

[ewbankkit]

Yes - see hashicorp/terraform-provider-awscc#222.

[paddycarver]

I think #213 meets these needs. Tests that I added should illustrate its behavior in various scenarios and explain what those scenarios are/why we decided to behave that way in those scenarios.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.