`UseStateForUnknown` doesn't preserve known `null` prior state values

[austinvalle]

Module version

v1.14.1 (latest)

Relevant provider source code

"computed_attr": schema.StringAttribute{
	Computed: true,
	PlanModifiers: []planmodifier.String{
		stringplanmodifier.UseStateForUnknown(),
	},
},

Expected Behavior

If the prior state value for computed_attr is null, that should be preserved and stored in the plan (if the attribute is unconfigured) because null is a known value.

What I believe the current logic is attempting to do, is prevent this plan modifier from running when the resource is creating (i.e. the entire state is null), which it is achieving, but a little too aggressively since it checks the specific attribute's state value instead:

terraform-plugin-framework/resource/schema/stringplanmodifier/use_state_for_unknown.go

Lines 39 to 42 in 15bac5c

	// Do nothing if there is no state value.
	if req.StateValue.IsNull() {
	return
	}

We should consider updating this logic to:

func (m useStateForUnknownModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) {
	// NOTE: This is the bug fix, do nothing if we are creating
	if req.State.Raw.IsNull() {
		return
	}

	// Do nothing if there is a known planned value.
	if !req.PlanValue.IsUnknown() {
		return
	}

	// Do nothing if there is an unknown configuration value, otherwise interpolation gets messed up.
	if req.ConfigValue.IsUnknown() {
		return
	}

	resp.PlanValue = req.StateValue
}

Actual Behavior

If the prior state value for computed_attr is null, an unknown value is being planned during update.

[austinvalle]

One of the impacts of fixing this bug would be any attributes in existing resource implementations that are relying on null values being set to unknown during update plans, which would now start to preserve the prior state (keeping null), so if the provider is not preserving those prior state values during apply, it could cause data consistency issues if that unknown marking was being relied on.

Off the top of my head, I'm not sure why you'd use UseStateForUnknown in this way, since the goal of this plan modifier has always been to denote an attribute that doesn't change during apply on updates. Fixing this bug doesn't change that goal, it just makes it more precise.

Regardless, it's worth considering a little deeper if that impact is negative in a way that we should consider introducing a new plan modifier instead of modifying this one.

---

I'm currently of the mind that we should just fix the bug in UseStateForUnknown across all of the schema plan modifiers.

[chrismarget-j]

From @austinvalle's initial description of this issue:

null is a known value

This seems like an important point for provider developers to understand. It's something I've tripped over a couple of times and I'm happy to have encountered this clear statement about it.

I used to have a poorly named helper function like this:

func IsKnown(v attr.Value) bool {
	return !v.IsUnknown() && !v.IsNull()
}

Something about chaining two double negatives (NOT not known AND NOT no value) was too hard for my brain every time I needed to make such a determination.

Somewhere along the way I renamed that function to HasValue() (I think that captures it better?), but I'm not sure I've fully internalized the lesson.

Anyway, I'm writing this comment both to thank you for inadvertently helping clarify this for me, and because dwelling here long enough to say hi might help me remember this fact next time I need it.

[austinvalle]

This fix has been released with v1.15.1 -> https://github.com/hashicorp/terraform-plugin-framework/releases/tag/v1.15.1