Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

types: Remove Attrs, AttrTypes, Elems, ElemTypes, Null, Unknown, and Value fields #523

Merged
merged 3 commits into from Oct 31, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Oct 25, 2022

Closes #447
Reference: #502

When the framework type system was originally being developed, the value types were introduced with exported fields which also served as the internal details of whether a value was null, unknown, or a known value of a friendlier Go type. It was known that there was the potential for issues, but the simplified developer experience seemed to outweigh the potential for developer issues. Fast forward a few months, this decision appears to have two consequences that the framework maintainers hear about across various forums.

One issue is that the value types directly expose their internal implementation details and support the three states of a Terraform type value: being null, unknown, or a known value. Only one state should ever be set, but provider developers can make a value that is any combination of those states. This makes the framework behavior potentially indeterminate from the provider developer perspective whether, for example, a null AND unknown value becomes null OR unknown as it works its way through the framework.

type ThingResourceModel struct{
  Computed types.String `tfsdk:"computed"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  var data ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Unknown value: types.String{Null: false, Unknown: true, Value: ""}
    "computed": plan.Computed,
  })

  // Maybe some external API responses here, but showing hardcoded updates for
  // brevity. This will make the value invalid by enabling Null without
  // disabling Unknown.
  data.Computed.Null = true

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Invalid value: types.String{Null: true, Unknown: true, Value: ""}
    "computed": data.Computed,
  })

  // The invalid value will be either null or unknown, depending on the
  // type implementation. If unknown, Terraform will error, since unknown
  // values are never allowed in state.
  resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
}

Another issue is that it is possible to create collection value types that do not match their type definition. This issue is especially more likely with types.Object where it is possible to accidentially omit attributes. While the framework handling would eventually catch this issue when dealing with the invalid value, it can be caught sooner.

// Invalid value (missing attribute and differing attribute name)
types.Object{
  AttrTypes: map[string]attr.Type{
    "one": types.StringType,
    "two": types.BoolType,
  },
  Attrs: map[string]attr.Value{
    "not_one": types.String{Value: "wrong name"},
  },
}

Another issue is that the default (zero-value) state for an "unset" value type turns into a known value, which is confusing since these values explicitly support being null. This causes Terraform errors which would surface to practitioners (especially when untested) that provider developers then have to troubleshoot the error message containing Terraform's type system details, potentially discover the reason why it is happening by looking at the framework type source code, then figure out a workable solution. It's not intuitive.

type ThingResourceModel struct{
  // let's assume this is left unconfigured (null in config and plan)
  Optional types.String `tfsdk:"optional"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  // Providers can opt to use a single variable that is updated based on an
  // external response, however that logic can be more difficult sometimes,
  // so it can be easier to split them. Showing the split way to exemplify
  // the "unset" problem.
  var plan, state ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)

  tflog.Trace(ctx, "Plan Values", map[string]any{
    // Null value: types.String{Null: true, Unknown: false, Value: ""}
    "optional": plan.Optional,
  })

  // Maybe some external API responses here, but intentionally not
  // doing any state.Optional setting, which might happen if the
  // external response for that data was null for example.

  tflog.Trace(ctx, "State Values", map[string]any{
    // Zero-value: types.String{Null: false, Unknown: false, Value: ""}
    "optional": state.Optional,
  })

  // The state zero-value will later cause Terraform to error, such as:
  // Error: Provider produced inconsistent result after apply
  // ... expected cty.NullVal(cty.String), got cty.StringVal("")
  // Since the plan value said it would be null.
  resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
}

This removal of the fields in preference of functions and methods aims to unexport the internal details and treat the value types as immutable once they are created. The zero-value implementations of these values is now null, instead of a known zero-value of the underlying type.

While not recommended for production usage without extensive testing, provider developers can opt for panic() inducing versions of collection value creations, rather than dealing with diag.Diagnostics everywhere.

Accessing value information after the migration can be accomplished with the following:

Prior Value Access New Value Access
(types.Bool).Value (types.Bool).ValueBool()
(types.Bool).Null (types.Bool).IsNull()
(types.Bool).Unknown (types.Bool).IsUnknown()
(types.Float64).Value (types.Float64).ValueFloat64()
(types.Float64).Null (types.Float64).IsNull()
(types.Float64).Unknown (types.Float64).IsUnknown()
(types.Int64).Value (types.Int64).ValueInt64()
(types.Int64).Null (types.Int64).IsNull()
(types.Int64).Unknown (types.Int64).IsUnknown()
(types.List).Elems (types.List).Elements() or (types.List).ElementsAs()
(types.List).ElemType (types.List).ElementType()
(types.List).Null (types.List).IsNull()
(types.List).Unknown (types.List).IsUnknown()
(types.Map).Elems (types.Map).Elements() or (types.Map).ElementsAs()
(types.Map).ElemType (types.Map).ElementType()
(types.Map).Null (types.Map).IsNull()
(types.Map).Unknown (types.Map).IsUnknown()
(types.Number).Value (types.Number).ValueBigFloat()
(types.Number).Null (types.Number).IsNull()
(types.Number).Unknown (types.Number).IsUnknown()
(types.Object).Attrs (types.Object).Attributes() or (types.Object).As()
(types.Object).AttrTypes (types.Object).AttributeTypes()
(types.Object).Null (types.Object).IsNull()
(types.Object).Unknown (types.Object).IsUnknown()
(types.Set).Elems (types.Set).Elements() or (types.Set).ElementsAs()
(types.Set).ElemType (types.Set).ElementType()
(types.Set).Null (types.Set).IsNull()
(types.Set).Unknown (types.Set).IsUnknown()
(types.String).Value (types.String).ValueString()
(types.String).Null (types.String).IsNull()
(types.String).Unknown (types.String).IsUnknown()

Creating values after the migration can be accomplished with the following:

Prior Value Creation New Value Creation
types.Bool{Value: /* value */} types.BoolValue(/* value */)
types.Bool{Null: true} types.BoolNull()
types.Bool{Unknown: true} types.BoolUnknown()
types.Float64{Value: /* value */} types.Float64Value(/* value */)
types.Float64{Null: true} types.Float64Null()
types.Float64{Unknown: true} types.Float64Unknown()
types.Int64{Value: /* value */} types.Int64Value(/* value */)
types.Int64{Null: true} types.Int64Null()
types.Int64{Unknown: true} types.Int64Unknown()
types.List{ElemType: /* element type */, Elems: /* value */} list, diags := types.ListValue(/* element type */, /* value */) or list, diags := types.ListValueFrom(context.Context, /* element type */, any) or list := types.ListValueMust(/* element type */, /* value */)
types.List{ElemType: /* element type */, Null: true} types.ListNull(/* element type */)
types.List{ElemType: /* element type */, Unknown: true} types.ListUnknown(/* element type */)
types.Map{ElemType: /* element type */, Elems: /* value */} m, diags := types.MapValue(/* element type */, /* value */) or m, diags := types.MapValueFrom(context.Context, /* element type */, any) or m := types.MapValueMust(/* element type */, /* value */)
types.Map{ElemType: /* element type */, Null: true} types.MapNull(/* element type */)
types.Map{ElemType: /* element type */, Unknown: true} types.MapUnknown(/* element type */)
types.Number{Value: /* value */} types.NumberValue(/* value */)
types.Number{Null: true} types.NumberNull()
types.Number{Unknown: true} types.NumberUnknown()
types.Object{AttrTypes: /* attribute types */, Attrs: /* attribute values */} object, diags := types.ObjectValue(/* attribute types */, /* attribute values */) or object, diags := types.ObjectValueFrom(context.Context, /* attribute types */, any) or object := types.ObjectValueMust(/* attribute types */, /* attribute values */)
types.Object{AttrTypes: /* attribute types */, Null: true} types.ObjectNull(/* attribute types */)
types.Object{AttrTypes: /* attribute types */, Unknown: true} types.ObjectUnknown(/* attribute types */)
types.Set{ElemType: /* element type */, Elems: /* value */} set, diags := types.SetValue(/* element type */, /* value */) or set, diags := types.SetValueFrom(context.Context, /* element type */, any) or set := types.SetValueMust(/* element type */, /* value */)
types.Set{ElemType: /* element type */, Null: true} types.SetNull(/* element type */)
types.Set{ElemType: /* element type */, Unknown: true} types.SetUnknown(/* element type */)
types.String{Value: /* value */} types.StringValue(/* value */)
types.String{Null: true} types.StringNull()
types.String{Unknown: true} types.StringUnknown()

…Value fields

Reference: #447
Reference: #502

When the framework type system was originally being developed, the value types were introduced with exported fields which also served as the internal details of whether a value was null, unknown, or a known value of a friendlier Go type. It was known that there was the potential for issues, but the simplified developer experience seemed to outweigh the potential for developer issues. Fast forward a few months, this decision appears to have two consequences that the framework maintainers hear about across various forums.

One issue is that the value types directly expose their internal implementation details and support the three states of a Terraform type value: being null, unknown, or a known value. Only one state should ever be set, but provider developers can make a value that is any combination of those states. This makes the framework behavior potentially indeterminate from the provider developer perspective whether, for example, a null AND unknown value becomes null OR unknown as it works its way through the framework.

```go
type ThingResourceModel struct{
  Computed types.String `tfsdk:"computed"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  var data ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Unknown value: types.String{Null: false, Unknown: true, Value: ""}
    "computed": plan.Computed,
  })

  // Maybe some external API responses here, but showing hardcoded updates for
  // brevity. This will make the value invalid by enabling Null without
  // disabling Unknown.
  data.Computed.Null = true

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Invalid value: types.String{Null: true, Unknown: true, Value: ""}
    "computed": data.Computed,
  })

  // The invalid value will be either null or unknown, depending on the
  // type implementation. If unknown, Terraform will error, since unknown
  // values are never allowed in state.
  resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
}
```

Another issue is that it is possible to create collection value types that do not match their type definition. This issue is especially more likely with `types.Object` where it is possible to accidentially omit attributes. While the framework handling would eventually catch this issue when dealing with the invalid value, it can be caught sooner.

```go
// Invalid value (missing attribute and differing attribute name)
types.Object{
  AttrTypes: map[string]attr.Type{
    "one": types.StringType,
    "two": types.BoolType,
  },
  Attrs: map[string]attr.Value{
    "not_one": types.String{Value: "wrong name"},
  },
}
```

Another issue is that the default (zero-value) state for an "unset" value type turns into a known value, which is confusing since these values explicitly support being null. This causes Terraform errors which would surface to practitioners (especially when untested) that provider developers then have to troubleshoot the error message containing Terraform's type system details, potentially discover the reason why it is happening by looking at the framework type source code, then figure out a workable solution. It's not intuitive.

```go
type ThingResourceModel struct{
  // let's assume this is left unconfigured (null in config and plan)
  Optional types.String `tfsdk:"optional"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  // Providers can opt to use a single variable that is updated based on an
  // external response, however that logic can be more difficult sometimes,
  // so it can be easier to split them. Showing the split way to exemplify
  // the "unset" problem.
  var plan, state ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)

  tflog.Trace(ctx, "Plan Values", map[string]any{
    // Null value: types.String{Null: true, Unknown: false, Value: ""}
    "optional": plan.Optional,
  })

  // Maybe some external API responses here, but intentionally not
  // doing any state.Optional setting, which might happen if the
  // external response for that data was null for example.

  tflog.Trace(ctx, "State Values", map[string]any{
    // Zero-value: types.String{Null: false, Unknown: false, Value: ""}
    "optional": state.Optional,
  })

  // The state zero-value will later cause Terraform to error, such as:
  // Error: Provider produced inconsistent result after apply
  // ... expected cty.NullVal(cty.String), got cty.StringVal("")
  // Since the plan value said it would be null.
  resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
}
```

This removal of the fields in preference of functions and methods aims to unexport the internal details and treat the value types as immutable once they are created. The zero-value implementations of these values is now null, instead of a known zero-value of the underlying type.

While not recommended for production usage without extensive testing, provider developers can opt for `panic()` inducing versions of collection value creations, rather than dealing with `diag.Diagnostics` everywhere.

Accessing value information after the migration can be accomplished with the following:

| Prior Value Access | New Value Access |
|--------------------|------------------|
| `(types.Bool).Value` | `(types.Bool).ValueBool()` |
| `(types.Bool).Null` | `(types.Bool).IsNull()` |
| `(types.Bool).Unknown` | `(types.Bool).IsUnknown()` |
| `(types.Float64).Value` | `(types.Float64).ValueFloat64()` |
| `(types.Float64).Null` | `(types.Float64).IsNull()` |
| `(types.Float64).Unknown` | `(types.Float64).IsUnknown()` |
| `(types.Int64).Value` | `(types.Int64).ValueInt64()` |
| `(types.Int64).Null` | `(types.Int64).IsNull()` |
| `(types.Int64).Unknown` | `(types.Int64).IsUnknown()` |
| `(types.List).Elems` | `(types.List).Elements()` or `(types.List).ElementsAs()` |
| `(types.List).ElemType` | `(types.List).ElementType()` |
| `(types.List).Null` | `(types.List).IsNull()` |
| `(types.List).Unknown` | `(types.List).IsUnknown()` |
| `(types.Map).Elems` | `(types.Map).Elements()` or `(types.Map).ElementsAs()` |
| `(types.Map).ElemType` | `(types.Map).ElementType()` |
| `(types.Map).Null` | `(types.Map).IsNull()` |
| `(types.Map).Unknown` | `(types.Map).IsUnknown()` |
| `(types.Number).Value` | `(types.Number).ValueBigFloat()` |
| `(types.Number).Null` | `(types.Number).IsNull()` |
| `(types.Number).Unknown` | `(types.Number).IsUnknown()` |
| `(types.Object).Attrs` | `(types.Object).Attributes()` or `(types.Object).As()` |
| `(types.Object).AttrTypes` | `(types.Object).AttributeTypes()` |
| `(types.Object).Null` | `(types.Object).IsNull()` |
| `(types.Object).Unknown` | `(types.Object).IsUnknown()` |
| `(types.Set).Elems` | `(types.Set).Elements()` or `(types.Set).ElementsAs()` |
| `(types.Set).ElemType` | `(types.Set).ElementType()` |
| `(types.Set).Null` | `(types.Set).IsNull()` |
| `(types.Set).Unknown` | `(types.Set).IsUnknown()` |
| `(types.String).Value` | `(types.String).ValueString()` |
| `(types.String).Null` | `(types.String).IsNull()` |
| `(types.String).Unknown` | `(types.String).IsUnknown()` |

Creating values after the migration can be accomplished with the following:

| Prior Value Creation | New Value Creation |
|----------------------|--------------------|
| `types.Bool{Value: /* value */}` | `types.BoolValue(/* value */)` |
| `types.Bool{Null: true}` | `types.BoolNull()` |
| `types.Bool{Unknown: true}` | `types.BoolUnknown()` |
| `types.Float64{Value: /* value */}` | `types.Float64Value(/* value */)` |
| `types.Float64{Null: true}` | `types.Float64Null()` |
| `types.Float64{Unknown: true}` | `types.Float64Unknown()` |
| `types.Int64{Value: /* value */}` | `types.Int64Value(/* value */)` |
| `types.Int64{Null: true}` | `types.Int64Null()` |
| `types.Int64{Unknown: true}` | `types.Int64Unknown()` |
| `types.List{ElemType: /* element type */, Elems: /* value */}` | `list, diags := types.ListValue(/* element type */, /* value */)` or `list, diags := types.ListValueFrom(context.Context, /* element type */, any)` or `list := types.ListValueMust(/* element type */, /* value */)` |
| `types.List{ElemType: /* element type */, Null: true}` | `types.ListNull(/* element type */)` |
| `types.List{ElemType: /* element type */, Unknown: true}` | `types.ListUnknown(/* element type */)` |
| `types.Map{ElemType: /* element type */, Elems: /* value */}` | `m, diags := types.MapValue(/* element type */, /* value */)` or `m, diags := types.MapValueFrom(context.Context, /* element type */, any)` or `m := types.MapValueMust(/* element type */, /* value */)` |
| `types.Map{ElemType: /* element type */, Null: true}` | `types.MapNull(/* element type */)` |
| `types.Map{ElemType: /* element type */, Unknown: true}` | `types.MapUnknown(/* element type */)` |
| `types.Number{Value: /* value */}` | `types.NumberValue(/* value */)` |
| `types.Number{Null: true}` | `types.NumberNull()` |
| `types.Number{Unknown: true}` | `types.NumberUnknown()` |
| `types.Object{AttrTypes: /* attribute types */, Attrs: /* attribute values */}` | `object, diags := types.ObjectValue(/* attribute types */, /* attribute values */)` or `object, diags := types.ObjectValueFrom(context.Context, /* attribute types */, any)` or `object := types.ObjectValueMust(/* attribute types */, /* attribute values */)` |
| `types.Object{AttrTypes: /* attribute types */, Null: true}` | `types.ObjectNull(/* attribute types */)` |
| `types.Object{AttrTypes: /* attribute types */, Unknown: true}` | `types.ObjectUnknown(/* attribute types */)` |
| `types.Set{ElemType: /* element type */, Elems: /* value */}` | `set, diags := types.SetValue(/* element type */, /* value */)` or `set, diags := types.SetValueFrom(context.Context, /* element type */, any)` or `set := types.SetValueMust(/* element type */, /* value */)` |
| `types.Set{ElemType: /* element type */, Null: true}` | `types.SetNull(/* element type */)` |
| `types.Set{ElemType: /* element type */, Unknown: true}` | `types.SetUnknown(/* element type */)` |
| `types.String{Value: /* value */}` | `types.StringValue(/* value */)` |
| `types.String{Null: true}` | `types.StringNull()` |
| `types.String{Unknown: true}` | `types.StringUnknown()` |
@bflad bflad added bug Something isn't working breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. labels Oct 25, 2022
@bflad bflad added this to the v0.16.0 milestone Oct 25, 2022
@bflad bflad requested a review from a team as a code owner October 25, 2022 20:53
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@bflad
Copy link
Member Author

bflad commented Oct 26, 2022

I think we'll be good to merge this in once v0.15.0 has baked for a little bit in the wild (maybe October 31st? 🎃 ), just to ensure there are no unexpected issues during the deprecation period that would require fixes on main.

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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