Improve diagnostics when types that cannot handle unknown or null are used #495
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,11 @@ import ( | |
| "math/big" | ||
| "reflect" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-go/tftypes" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-framework/attr" | ||
| "github.com/hashicorp/terraform-plugin-framework/diag" | ||
| "github.com/hashicorp/terraform-plugin-framework/path" | ||
| ) | ||
|
|
||
| // Into uses the data in `val` to populate `target`, using the reflection | ||
|
|
@@ -22,19 +23,20 @@ import ( | |
| // in the tftypes.Value must have a corresponding property in the struct. Into | ||
| // will be called for each struct field. Slices will have Into called for each | ||
| // element. | ||
| func Into(ctx context.Context, typ attr.Type, val tftypes.Value, target interface{}, opts Options, path path.Path) diag.Diagnostics { | ||
| var diags diag.Diagnostics | ||
|
|
||
| v := reflect.ValueOf(target) | ||
| if v.Kind() != reflect.Ptr { | ||
| err := fmt.Errorf("target must be a pointer, got %T, which is a %s", target, v.Kind()) | ||
| diags.AddAttributeError( | ||
| path, | ||
| "Value Conversion Error", | ||
| fmt.Sprintf("An unexpected error was encountered trying to convert the value. This is always an error in the provider. Please report the following to the provider developer:\n\nPath: %s\nError: %s", path.String(), err.Error()), | ||
| ) | ||
| return diags | ||
| } | ||
| result, diags := BuildValue(ctx, typ, val, v.Elem(), opts, path) | ||
| if diags.HasError() { | ||
| return diags | ||
| } | ||
|
|
@@ -109,11 +111,12 @@ func BuildValue(ctx context.Context, typ attr.Type, val tftypes.Value, target re | |
| // all that's left to us now is to set it as an empty value or | ||
| // throw an error, depending on what's in opts | ||
| if !opts.UnhandledUnknownAsEmpty { | ||
|
There was a problem hiding this comment. 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 I would expect (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. There was a problem hiding this comment. 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 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 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:
// 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 conditionI 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? There was a problem hiding this comment. To be a little clearer, I was understanding this 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
In the above I'm making the assumption that 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
}There was a problem hiding this comment.
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. There was a problem hiding this comment. I created #498 to followup on this. 👍 Thank you for raising this here. |
||
| diags.AddAttributeError( | ||
| path, | ||
| "Value Conversion Error", | ||
| "An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:\n\n"+ | ||
| "Received unknown value, however the target type cannot handle unknown values. Use the corresponding `types` package type or a custom type that handles unknown values.\n\n"+ | ||
| fmt.Sprintf("Path: %s\nTarget Type: %s\nSuggested Type: %s", path.String(), target.Type(), typ.String()), | ||
| ) | ||
| return target, diags | ||
| } | ||
|
|
@@ -132,12 +135,14 @@ func BuildValue(ctx context.Context, typ attr.Type, val tftypes.Value, target re | |
| return reflect.Zero(target.Type()), nil | ||
| } | ||
|
|
||
| diags.AddAttributeError( | ||
| path, | ||
| "Value Conversion Error", | ||
| "An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:\n\n"+ | ||
| "Received null value, however the target type cannot handle null values. Use the corresponding `types` package type, a pointer type or a custom type that handles null values.\n\n"+ | ||
| fmt.Sprintf("Path: %s\nTarget Type: %s\nSuggested `types` Type: %s\nSuggested Pointer Type: *%s", path.String(), target.Type(), typ.String(), target.Type()), | ||
| ) | ||
|
|
||
| return target, diags | ||
| } | ||
| // *big.Float and *big.Int are technically pointers, but we want them | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the
path.Pathinformation is available in this function, we should replace thediags.AddError()call below withdiags.AddAttributeError()and consider addingPath: path.String()\nError: err.Error()to the details.