types: Consider changing value types zero-values to null or unknown

[PJB3005]

Apologies if this issue is a bit overzealous. I am converting our existing Terraform provider over to Plugin Framework (since the provider is only half-finished anyways) and this is some feedback I hope may be useful.

Module version

v0.10.0

Use-cases

Types like types.String use an explicit Null bool and Unknown bool value to indicate that they are not-valid values. This is problematic because a zero-initialized default is actually considered valid.

If I am writing an implementation of a resource, I may have a schema and model like so:

"id": {
	Type: types.StringType,
	Computed: true,
},
"foo": {
	Type:     types.StringType,
	Computed: true,
},

type Bar struct {
	ID  types.String `tfsdk:"id"`
	Foo types.String `tfsdk:"foo"`

And I fill it like so:

bar, err := r.p.client.CreateBar(items)

state.ID = types.String{Value: bar.ID}
// Oops, forgot to set Foo!

diags = resp.State.Set(ctx, result)

Now my foo attribute wasn't set. Because the default value of types.String is a valid, non-null empty string, that is now the default value of my attribute. If the struct were re-organized so that types.String{} is unknown, this above code would give an obvious error at apply time (since the provider gives back an unknown value, which is illegal).

This is only a problem for Computed properties, since "" is probably not the expected value in that case and Terraform will complain that the provider gave back an invalid value.

Furthermore, it is somewhat awkward that these are two fields are mutually exclusive. i.e. it is not legal to have both Null: true and Unknown: true. This is somewhat minor, however.

(Also dealing with these types in general is already really annoying, though I suppose you can't do much better in Go...)

Attempted Solutions

None

Proposal

To fix this, I would re-arrange these types like this:

const StateUnknown int = 0
const StateNull    int = 1
const StateValue   int = 2

type String struct {
	State int
	Value string 
}

// IsNull returns true if the String represents a null value.
func (s String) IsNull() bool {
	return s.State == StateNull
}

// IsUnknown returns true if the String represents a currently unknown value.
func (s String) IsUnknown() bool {
	return s.State == StateUnknown
}

// IsValue returns true if the String is not null or unknown
func (s String) IsValue() bool {
	return s.State == StateValue
}

This does make it more annoying to construct these values though. It might make sense to define helper functions like this as well:

func NewString(value string) String {
	return String{Value: value, State: StateValue}
}

func UnknownString() String {
	return String{}
}

func NullString() String {
	return String{State: StateNull}
}

Typing out these function calls would actually be shorter than the current system, but only by a few characters.

Furthermore, it opens up the door to further helper functions like this, which I've personally been defining myself in our own provider and think are very useful:

func NewStringP(value *string) String {
	if value == nil {
		return String{State: StateNull}
	}

	return String{Value: *value, State: StateValue}
}

References

None that I could find.

[bflad]

Hi @PJB3005 👋 Thank you for submitting this. Generally, really good ideas here.

This came up in https://github.com/hashicorp/terraform-plugin-framework/pull/495#pullrequestreview-1118745112 as something to consider if/when we want to refactor the types package. While I'm not sure that we should necessarily default to any particular value kind (null, unknown, or known) since that could introduce its own potential for confusion, there are similar ideas we can use from your proposal to prevent provider developers from creating "invalid" values, which could involve function calls.

Essentially the proposal would be to (at least) unexport the Null, Unknown, and Value fields on the existing types value types and require value creation through functions such as:

func NullBool() Bool
func NullFloat64() Float64
func NullInt64() Int64
func NullList() List
func NullMap() Map
func NullNumber() Number
func NullObject() Object
func NullSet() Set
func NullString() String

func UnknownBool() Bool
func UnknownFloat64() Float64
func UnknownInt64() Int64
func UnknownList() List
func UnknownMap() Map
func UnknownNumber() Number
func UnknownObject() Object
func UnknownSet() Set
func UnknownString() String

func BoolValue(bool) Bool
func Float64Value(float64) Float64
func Int64Value(int64) Int64
func ListValue([]attr.Value) List
func MapValue(map[string]attr.Value) Map
func NumberValue(*big.Float) Number
func ObjectValue(struct) Object
func SetValue([]attr.Value) Set
func StringValue(string) String
// Potentially others, if they make sense.

For creating lists, maps, objects, and sets, this has the additional benefit of not requiring the inline definition of the element/attribute types, since they can be derived from the values.

To access "friendlier" Go types of the value, there could be through methods such as:

func (b Bool) BoolValue() bool
func (f Float64) Float64Value() float64 // could consider Float32Value(), etc.
func (i Int64) Int64Value() int64 // could consider IntValue(), etc.
func (n Number) BigFloatValue() *big.Float // could consider Float64Value() with errors, etc.
func (s String) StringValue() string
// refer to existing collection type methods, such as As()

These could return a zero-value of the type in the case it happens to be null or unknown. Determining whether a value is null or unknown can be done with the IsNull() bool and IsUnknown() bool interface methods that are already required on the value types.

Whether the internal implementation then becomes some sort of state field as you recommend or an even fancier generics implementation as mentioned in the link above, I think is fairly moot unless we want to somehow impose this implementation detail as a requirement for provider-defined types. I think that can be handled separately, but curious if you feel otherwise.

---

One particular thing to note here, which in your description is teased about here and I think is worth its own issue for tracking, is that the framework currently relies on Terraform core to return feedback that a value is unknown when writing the state, which is never allowed. The framework can (and probably should) catch this data handling issue after the provider-definied logic has finished and return its own, verbose error diagnostic that can explain the error and what to do about it for provider developers.

---

Thanks again!

[bflad]

Just to expand on this, since I'm going through other similar issues at the moment:

While I'm not sure that we should necessarily default to any particular value kind (null, unknown, or known) since that could introduce its own potential for confusion, there are similar ideas we can use from your proposal to prevent provider developers from creating "invalid" values, which could involve function calls.

There are in fact quite a few options:

• Treat wholly unset value types (zero-value) as unknown, as suggested here, and rely on the later mentioned unknown value errors.
• Treat wholly unset value types (zero-value) as null, which might better represent desirable provider developer logic ("I never set this because I never received a value for it"), but might cause other confusing Terraform core errors if value rules are not followed correctly.
• Treat wholly unset value types (zero-value) as invalid and raise an error similar to the mentioned unknown value error.

Using something like a "value state" where the zero-value for the type makes it one of these is likely a good implementation choice. As you may be able to tell though, all of these have their own tradeoffs, unfortunately. Many of these errors are runtime specific, so it is a longer feedback loop. I'm not quite sure how we can avoid that.

I think I actually lean towards null here though, since it feels like it represents the best zero-value choice for the purpose of the type. I think the rules surrounding null values may be a little clearer to understand, even if Terraform core catches it after we send the response (e.g. "expected {known value}, got null" type errors) It would also prevent the need for introducing support for pointers to types types in the internal reflection code, so folks can implement null-on-unset behaviors in their logic.

[github-actions[bot]]

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.