attr: Consider Adding IsNull() and IsUnknown() methods to Value

[bflad]

Module version

v0.4.2

Use-cases

When writing attribute plan modifiers, it can be necessary to check if the configuration or state attr.Value are null or unknown. It feels a little tedious to have to know the underlying value type for comparison and how to properly create the conditional.

Attempted Solutions

"function_name": {
			// Property: FunctionName
			// CloudFormation resource type schema:
			// {
			//   "description": "The name of the Lambda function, up to 64 characters in length. If you don't specify a name, AWS CloudFormation generates one.",
			//   "minLength": 1,
			//   "type": "string"
			// }
			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.RequiresReplaceIf(func(ctx context.Context, state, config attr.Value, path *tftypes.AttributePath) (bool, diag.Diagnostics) {
					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"),
			},
		},

Proposal

type Value interface {
	// ... existing methods ...

	// IsNull returns true if the value is null.
	IsNull() bool

	// IsUnknown returns true if the value is unknown.
	IsUnknown() bool
}

For any attr.Value that potentially do not understand null/unknown, they can always return false for both methods. The implementation above then becomes:

			PlanModifiers: []tfsdk.AttributePlanModifier{
				tfsdk.RequiresReplaceIf(func(ctx context.Context, state, config attr.Value, path *tftypes.AttributePath) (bool, diag.Diagnostics) {
					if config.Equal(state) || config.IsNull() {
						return false, nil
					}

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

[bflad]

Another use case is attempting to write a generic state -> plan attribute plan modifier for Computed attributes:

type computedValueDoesNotChangeModifier struct {}

func (m computedValueDoesNotChangeModifier) Description(ctx context.Context) string {
	return "Value does not change after creation."
}

func (m computedValueDoesNotChangeModifier) MarkdownDescription(ctx context.Context) string {
	return m.Description()
}

func (m computedValueDoesNotChangeModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, resp *tfsdk.ModifyAttributePlanResponse) {
	// Do nothing for Create
	if req.AttributeState == nil || req.AttributeState.IsNull() {
		return
	}

	resp.AttributePlan = req.AttributeState
}

[paddycarver]

I think we talked about this previously, and discarded the idea because it opened the door for inconsistent implementations (i.e., IsNull() returns false but ToTerraformValue returns nil.

Does a tfsdk.ValueIsNull() and tfsdk.ValueIsUnknown() address this?

[bflad]

I think we talked about this previously, and discarded the idea because it opened the door for inconsistent implementations (i.e., IsNull() returns false but ToTerraformValue returns nil.

What if we recommend the ToTerraformValue() implementation if IsNull() would otherwise be complicated to implement (e.g. its not just a field on the custom type)?

Does a tfsdk.ValueIsNull() and tfsdk.ValueIsUnknown() address this?

Likely, since I presume the implementation would be something like the following:

func ValueIsNull(ctx context.Context, val attr.Value) (bool, error) {
  tfValRaw, err := val.ToTerraformValue(ctx)

  return tfValRaw == nil, err
}

That implementation does require more gymnastics though since you either have to pass back the error, making it not a simple conditional helper, or as a cursed implementation, swallow the error.

How discoverable are these tfsdk methods though, especially as the package is continuing to grow in surface area? As a provider developer, how do I know to look in the tfsdk package for these when I'm working with attr.Value in the documentation or language server hints? Maybe this would be less of a mental hurdle (and tfsdk less of a "grab bag" Go package) if Value handling helpers were in their own package that would be easy to reference as a whole.

[bflad]

I also think that encouraging or requiring custom implementations to explicitly think about null and unknown values, e.g. by introducing these methods, might make for overall less problematic implementations such as those only backed by built-in, non-pointer Go types since a developer would need to 🤔 whether what they are doing might be problematic. I guess I lean towards prioritizing ease of use, since this is likely a common use case, over potential risks with inconsistent custom implementations.

[paddycarver]

That implementation does require more gymnastics though since you either have to pass back the error, making it not a simple conditional helper, or as a cursed implementation, swallow the error.

I'm debating the merits of refining that method's interface to remove the error, and having types panic if there's an error. How can that error be handled, really? What's can be done with it by returning it? Is it a better provider developer experience to panic and see exactly where the error is, rather than returning it and potentially having to trace it back through? Given we're talking about changing ToTerraformValue to return a tftypes.Value (#171) and creating a tftypes.Value panics on wrong input, does it make sense for ToTerraformValue to just panic? 🤔

Does that change any of the calculus on this?

How discoverable are these tfsdk methods though, especially as the package is continuing to grow in surface area? As a provider developer, how do I know to look in the tfsdk package for these when I'm working with attr.Value in the documentation or language server hints? Maybe this would be less of a mental hurdle (and tfsdk less of a "grab bag" Go package) if Value handling helpers were in their own package that would be easy to reference as a whole.

This is a solid point and I'm inclined to agree, but I think we're due a refactoring of our package hierarchy anyways. There are some odd dependency paths here that just need mapped out so we can figure out what we can safely split out.

I guess I lean towards prioritizing ease of use, since this is likely a common use case, over potential risks with inconsistent custom implementations.

I think we're talking about what use is going to suffer, here. On one hand, we can make checking values easier. But doing so makes implementing types correctly more difficult and leaves more opportunities for errors.

I don't know that I disagree with you that "making types" is the right place to locate this difficulty--I've definitely sided with that being the more advanced use case and therefore making it responsible for more complexity in the past--I think I'm just 🤔 about whether we need to make either of these harder, or if we can find a solution that is easy to use without making implementations more difficult. I'm suspicious we can, but I do think it would require some (light) refactoring.

But yes, ideally these would end up in the attr package, but I believe that creates import cycles right now. attr.IsNull and attr.IsUnknown were my first idea, and then I implemented it, and import cycles :( And that error definitely complicates that rosy picture. :(

[bflad]

Partially related: #201 and #221

[bflad]

After #231, the ToTerraformValue() method on attr.Values can be used to get a tftypes.Value directly, which has IsNull() and IsKnown()methods.

[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.