Improve diagnostics when types that cannot handle unknown or null are used

[bendbennett]

Closes: #191

[bflad]

Seeing the missing path information here, I think we should update reflect.Into() to accept a path.Path parameter. This would cover GetAttribute(), etc. usage and allow us to remove some diag.WithPath() usage, which was a workaround. There's going to be a lot of other reflection code that won't have the path readily available still, so I think setting those calls to path.Empty() for now is okay, and we can see about potentially improving those later.

[bendbennett]

I've updated reflect.Into() to accept path.Path. I'm a little unsure which diag.WithPath() you're referring to removing so I have left these in place for now.

[bflad]

Is this necessary for anonymous structs types? Given an attr.Type, I think we can just call typ.String() and anticipate folks implementing their types correctly, rather than potentially returning some confusing type details.

[bendbennett]

I've removed this in both unknown and null cases and have switched to using typ.String() as you suggested.

[bflad]

Almost there!

[bflad]

Here's the diag.WithPath() handling that can now be removed since the above now correctly passed the path information. 👍

[bflad]

Ah, okay. The suggested type here should be types.Bool for now, but stripping the Type suffix would only temporarily work for the framework types types and isn't a long term solution. The attr.Type interface doesn't give us a way to really know the correct/suggested attr.Value type and maybe it should. Technically, we could use the attr.Value returned from the ValueFromTerraform method if called with a null value of the TerraformType, but custom types don't have to support null values if they're always required 🙁 . I'll create an issue to consider adding something to that interface so we can recommend things correctly.

For now though, I'm not sure if there's okay enough messaging we can give here to say "use the value type associated with the type". We might be able to merge this in as-is right now and try to fix that up before the next release.

[bendbennett]

We could alter the wording in the diagnostic until attr: Consider Adding ValueType Method to Type Interface is addressed.

Perhaps something like:

Path: path.String()
Target Type: target.Type()
Suggested `types` Type: typ.String() (use the value type associated with the type e.g., use types.Object for types.ObjectType)
Suggested Pointer Type: *target.Type()

[bflad]

Now that the path.Path information is available in this function, we should replace the diags.AddError() call below with diags.AddAttributeError() and consider adding Path: path.String()\nError: err.Error() to the details.

[bflad]

Looks good to me 🚀 (let's follow up with #496)

[apparentlymart]

I'm a little concerned that this particular design may behave as a confusing trap for provider developers, for the reason I shared over in #191 (comment) .

Specifically: whether a value is known or not during planning is under the control of the module author, not the provider developer. Therefore I think it's technically invalid to decode any plan-time value into a type that cannot support unknowns, regardless of whether the value happens to actually be unknown in a particular request.

Do you think it would be reasonable to add (in another PR of course, since I see this one is already closed!) an additional flag to opts like RequireHandlingUnknown bool, and then if that's set then we'd always return an error if the target type can't handle unknowns, regardless of whether the given val happens to be unknown or not?

I would expect RequireUnknownHandling to be true for any call that happens while the framework is handling a PlanResourceChange request, one of the several "validate" RPCs, and ConfigureProvider, because those are all situations where the module author can compel the provider to deal with unknown values. This restriction would not be required for ApplyResourceChange, ReadResource, or ReadDataSource, because Terraform Core guarantees that everything will be known by the time those are called. Therefore this additional restriction would not affect the most common case where a provider is only implementing apply-time logic and letting the framework's own logic handle the validation and planning steps.

(A more accurate way to say it is: "config" and "proposed new value" can always potentially contain unknowns, but prior state should never.)

My motivation here is to let provider developers know during their initial development that they need to handle this case, rather than them "getting away with it" because their test cases happen to all have hard-coded known values and then only later getting bug reports from their users when they pass in an unknown value and get this error. I assume provider developers would rather know about this requirement early, rather than needing to rework their logic later only once an end-user discovers it.

[bflad]

I'd personally be worried about introducing an option, which if treated as opt-in, that developers won't opt into or know to opt into. Most of our current data handling documentation glosses over some of the available options and for most developers they won't encounter them until they are using the framework-defined types collection types, which usually support them in methods like As(), ElementsAs(), etc.

Admittedly, this is probably the most confusing aspect of provider development with the framework. Very regularly, we see developers struggle with these concepts in Discuss, etc. The associated website documentation is quite confusing where it tries to describe all the various rules. Our best advice is typically to use the framework-defined types types, but its so easy to reach for standard Go types, slices of provider-defined types in the case of lists/sets, and structs (generally without pointers) of provider-defined types in the case of objects. As you can imagine, these all run into the trouble you mention.

I wonder if it may be better if we took a more wholesale step back and re-evaluated the type system to ensure correctness, while still supporting some of the "nice-ities" of the current type system. We have been circling the idea of requiring function calls for interacting with values, rather than accessing/setting struct fields to cover #447. Combining that with an always-enabled rule like you mention in the internal reflection code, might do the trick?

I only mention combined these right now because we're running very low on time to make substantial changes to the framework before a first major version is released and its easier to consider the whole subsystem at once.

A quick sketch of these combined ideas might be:

• Requiring all framework-defined types or custom provider-defined types that interact with schema-based data must always support null and unknown values.
• Requiring the creation of framework-defined value types through function calls so there is no method for creating invalid values.
• Requiring the access of framework-defined value types through method calls which can default to zero-values if null/unknown (NOTE: the attr.Value interface already requires IsNull() bool and IsUnknown() bool methods, so there's no need to access any sort of weird pointer or sentinel value representations)

// Creating framework-defined value types

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

// Access framework-defined value type values

func (b Bool) BoolValue() b
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()

// provider-defined types

tfsdk.Schema{
  Attributes: map[string]tfsdk.Attribute{
    "example: {
      // One of Computed, Optional, Required: true, doesn't matter
      Type: types.StringType,
     },
  },
}

type A struct {
  Example string `tfsdk:"example"`
}

type B struct {
  Example *string `tfsdk:"example"`
}

type B struct {
  Example types.String `tfsdk:"example"`
}

var (
  a A
  b B
  c C
  d string
  e *string
  f types.String
)

(tfsdk.Config).Get(ctx, &a) // Always errors
(tfsdk.Config).Get(ctx, &b) // Always errors
(tfsdk.Config).Get(ctx, &c) // No error for this condition
(tfsdk.Config).GetAttribute(ctx, path.Root("example"), &d) // Always errors
(tfsdk.Config).GetAttribute(ctx, path.Root("example"), &e) // Always errors
(tfsdk.Config).GetAttribute(ctx, path.Root("example"), &f) // No error for this condition

(tfsdk.Plan).Get(ctx, &a) // Always errors
(tfsdk.Plan).Get(ctx, &b) // Always errors
(tfsdk.Plan).Get(ctx, &c) // No error for this condition
(tfsdk.Plan).GetAttribute(ctx, path.Root("example"), &d) // Always errors
(tfsdk.Plan).GetAttribute(ctx, path.Root("example"), &e) // Always errors
(tfsdk.Plan).GetAttribute(ctx, path.Root("example"), &f) // No error for this condition

(tfsdk.State).Get(ctx, &a) // Always errors
(tfsdk.State).Get(ctx, &b) // Always errors
(tfsdk.State).Get(ctx, &c) // No error for this condition
(tfsdk.State).GetAttribute(ctx, path.Root("example"), &d) // Always errors
(tfsdk.State).GetAttribute(ctx, path.Root("example"), &e) // Always errors
(tfsdk.State).GetAttribute(ctx, path.Root("example"), &f) // No error for this condition

I think we can still be lenient when setting values as long as they follow the current reflection implementation.

Does this align with what you would expect?

[apparentlymart]

To be a little clearer, I was understanding this reflect.Into function as being an internal implementation detail of the higher level API for fetching values from the config, plan, and state, and so I was therefore assuming that the "options" here are always generated by the framework's own code rather than from provider code. If that isn't true then I would agree that the details of what I proposed don't make sense!

I defer to you on the API design details since I'm still very new to the framework design! I think the scenarios you wrote out here seem to make sense as far as I can mentally evaluate them, although I think it may be possible to make the tfsdk.State API be more permissive and fail only if the specific given value happens to be wrong, because:

• tfsdk.State should never encounter unknown values, I think?
• By the time we're dealing in state, the provider is able to control whether null values are valid or not and so can in principle use Required in the schema or validation and/or plan-time rules to ensure that they'll never need to deal with null in a particular argument.

In the above I'm making the assumption that tfsdk.State is only used to describe final state values, such as what would be passed in as the prior state to ReadResource and PlanResourceChange, and what would be returned from ApplyResourceChange. It would be trickier if we ever use tfsdk.State to represent a "proposed new state" or a "planned new state", because of course those can potentially contain unknown values if the configuration contains unknown values.

On the other hand, I think it could potentially be reasonable to argue that it's better to be consistent and have the same rules everywhere, so that there's the most opportunity for code reuse. Particularly with Go 1.18 type parameters it seems possible to write a generic type that can represent a possibly-null, possibly-unknown value of any Go type so that there doesn't need to be a custom type for every combination, which may lower the burden of using these enough that it's reasonable to just make them always required. 🤔

type ValueState int

const (
    ValueConcrete ValueState = iota
    ValueNull
    ValueUnknown
)

// (I don't know what would be a good name for this type,
// so this is just a placeholder.)
type SomethingValue[T any] {
    Value T
    State ValueState
}

[bflad]

On the other hand, I think it could potentially be reasonable to argue that it's better to be consistent and have the same rules everywhere, so that there's the most opportunity for code reuse.

This is the direction that feels right to me personally -- having inconsistent ways to interact with "schema-based data" between the various RPCs, attribute configurability, and type of data (config, plan, state) makes for harder comprehension, documentation, etc. for provider developers. We can certainly document Terraform's general implementation details there if developers want to potentially shortcut some logic (like checking for unknown values when there cannot be any), but I think there are diminishing returns by trying too hard to support Go standard library types or provider-defined slices/structs directly in the reflection when they can be incompatible outside the control of the provider developer.

I will say, it's been very nice to do things like this even in the current framework design:

type exampleResourceModel struct {
  ExampleList types.List `tfsdk:"example_list"`
  ID          types.String `tfsdk:"id"`
}

func (r exampleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
	var data exampleResourceModel

	// Read plan data into model
	resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)

	if resp.Diagnostics.HasError() {
		return
	}

	// Potentially interact with infrastructure

	// Set any Computed/Unknown values
	data.ID = types.String{Value: "example"}

	// Write state data from model
	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
}

It's easy to document on our side and a fairly succinct implementation, code-wise, in my opinion.

[bflad]

I created #498 to followup on this. 👍 Thank you for raising this here.

[github-actions]

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