Skip to content

Commit

Permalink
types: Deprecate Null, Unknown, and Value fields
Browse files Browse the repository at this point in the history 
Reference: #447

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 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 deprecation 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. When provider developers switch over to the new model, any errant changes to the deprecated exported fields will have no effect. A future version will remove the exported fields entirely and switch the zero-value implementation of these values to being null, instead of a known zero-value of the underlying type.

Update CHANGELOG for #502

types: Note that valueState constants will be exported in the future so they can be used by external types

types: collection types
  • Loading branch information
@bflad
bflad committed Oct 21, 2022
1 parent 2be6665 commit 4dfa83c
Show file tree
Hide file tree
Showing 63 changed files with 6,383 additions and 1,260 deletions.
95 changes: 95 additions & 0 deletions .changelog/502.txt
View file
@@ -0,0 +1,95 @@
```release-note:note
types: The `Bool` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `BoolNull()`, `BoolUnknown()`, and `BoolValue()` creation functions and `IsNull()`, `IsUnknown()`, and `ValueBool()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `Float64` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `Float64Null()`, `Float64Unknown()`, and `Float64Value()` creation functions and `IsNull()`, `IsUnknown()`, and `ValueFloat64()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `Int64` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `Int64Null()`, `Int64Unknown()`, and `Int64Value()` creation functions and `IsNull()`, `IsUnknown()`, and `ValueInt64()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `List` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `ListNull()`, `ListUnknown()`, and `ListValue()` creation functions and `Elements()`, `ElementsAs()`, `ElementType()`, `IsNull()`, and `IsUnknown()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `Map` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `MapNull()`, `MapUnknown()`, and `MapValue()` creation functions and `Elements()`, `ElementsAs()`, `ElementType()`, `IsNull()`, and `IsUnknown()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `Number` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `NumberNull()`, `NumberUnknown()`, and `NumberValue()` creation functions and `IsNull()`, `IsUnknown()`, and `ValueBigFloat()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `Set` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `SetNull()`, `SetUnknown()`, and `SetValue()` creation functions and `Elements()`, `ElementsAs()`, `ElementType()`, `IsNull()`, and `IsUnknown()` methods. The fields will be removed in a future release.
```

```release-note:note
types: The `String` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `StringNull()`, `StringUnknown()`, and `StringValue()` creation functions and `IsNull()`, `IsUnknown()`, and `ValueString()` methods. The fields will be removed in a future release.
```

```release-note:enhancement
types: Added `BoolNull()`, `BoolUnknown()`, `BoolValue()` functions, which create immutable `Bool` values
```

```release-note:enhancement
types: Added `Float64Null()`, `Float64Unknown()`, `Float64Value()` functions, which create immutable `Float64` values
```

```release-note:enhancement
types: Added `Int64Null()`, `Int64Unknown()`, `Int64Value()` functions, which create immutable `Int64` values
```

```release-note:enhancement
types: Added `ListNull()`, `ListUnknown()`, `ListValue()` functions, which create immutable `List` values
```

```release-note:enhancement
types: Added `MapNull()`, `MapUnknown()`, `MapValue()` functions, which create immutable `Map` values
```

```release-note:enhancement
types: Added `NumberNull()`, `NumberUnknown()`, `NumberValue()` functions, which create immutable `Number` values
```

```release-note:enhancement
types: Added `SetNull()`, `SetUnknown()`, `SetValue()` functions, which create immutable `Set` values
```

```release-note:enhancement
types: Added `StringNull()`, `StringUnknown()`, `StringValue()` functions, which create immutable `String` values
```

```release-note:enhancement
types: Added `Bool` type `ValueBool()` method, which returns the `bool` of the known value or `false` if null or unknown
```

```release-note:enhancement
types: Added `Float64` type `ValueFloat64()` method, which returns the `float64` of the known value or `0.0` if null or unknown
```

```release-note:enhancement
types: Added `Int64` type `ValueInt64()` method, which returns the `int64` of the known value or `0` if null or unknown
```

```release-note:enhancement
types: Added `List` type `Elements()` method, which returns the `[]attr.Value` of the known values or `nil` if null or unknown
```

```release-note:enhancement
types: Added `Map` type `Elements()` method, which returns the `map[string]attr.Value` of the known values or `nil` if null or unknown
```

```release-note:enhancement
types: Added `Number` type `ValueBigFloat()` method, which returns the `*big.Float` of the known value or `nil` if null or unknown
```

```release-note:enhancement
types: Added `Set` type `Elements()` method, which returns the `[]attr.Value` of the known values or `nil` if null or unknown
```

```release-note:enhancement
types: Added `String` type `ValueString()` method, which returns the `string` of the known value or `""` if null or unknown
```
19 changes: 11 additions & 8 deletions internal/fwserver/attr_value.go
View file
Expand Up @@ -68,15 +68,16 @@ func listElemObject(ctx context.Context, schemaPath path.Path, list types.List,
return listElemObjectFromTerraformValue(ctx, schemaPath, list, description, tftypes.UnknownValue)
}

if index >= len(list.Elems) {
if index >= len(list.Elements()) {
return listElemObjectFromTerraformValue(ctx, schemaPath, list, description, nil)
}

return coerceObjectValue(schemaPath, list.Elems[index])
return coerceObjectValue(schemaPath, list.Elements()[index])
}

func listElemObjectFromTerraformValue(ctx context.Context, schemaPath path.Path, list types.List, description fwschemadata.DataDescription, tfValue any) (types.Object, diag.Diagnostics) {
elemValue, err := list.ElemType.ValueFromTerraform(ctx, tftypes.NewValue(list.ElemType.TerraformType(ctx), tfValue))
elemType := list.ElementType(ctx)
elemValue, err := elemType.ValueFromTerraform(ctx, tftypes.NewValue(elemType.TerraformType(ctx), tfValue))

if err != nil {
return types.Object{Null: true}, diag.Diagnostics{
Expand All @@ -96,7 +97,7 @@ func mapElemObject(ctx context.Context, schemaPath path.Path, m types.Map, key s
return mapElemObjectFromTerraformValue(ctx, schemaPath, m, description, tftypes.UnknownValue)
}

elemValue, ok := m.Elems[key]
elemValue, ok := m.Elements()[key]

if !ok {
return mapElemObjectFromTerraformValue(ctx, schemaPath, m, description, nil)
Expand All @@ -106,7 +107,8 @@ func mapElemObject(ctx context.Context, schemaPath path.Path, m types.Map, key s
}

func mapElemObjectFromTerraformValue(ctx context.Context, schemaPath path.Path, m types.Map, description fwschemadata.DataDescription, tfValue any) (types.Object, diag.Diagnostics) {
elemValue, err := m.ElemType.ValueFromTerraform(ctx, tftypes.NewValue(m.ElemType.TerraformType(ctx), tfValue))
elemType := m.ElementType(ctx)
elemValue, err := elemType.ValueFromTerraform(ctx, tftypes.NewValue(elemType.TerraformType(ctx), tfValue))

if err != nil {
return types.Object{Null: true}, diag.Diagnostics{
Expand Down Expand Up @@ -156,15 +158,16 @@ func setElemObject(ctx context.Context, schemaPath path.Path, set types.Set, ind
return setElemObjectFromTerraformValue(ctx, schemaPath, set, description, tftypes.UnknownValue)
}

if index >= len(set.Elems) {
if index >= len(set.Elements()) {
return setElemObjectFromTerraformValue(ctx, schemaPath, set, description, nil)
}

return coerceObjectValue(schemaPath, set.Elems[index])
return coerceObjectValue(schemaPath, set.Elements()[index])
}

func setElemObjectFromTerraformValue(ctx context.Context, schemaPath path.Path, set types.Set, description fwschemadata.DataDescription, tfValue any) (types.Object, diag.Diagnostics) {
elemValue, err := set.ElemType.ValueFromTerraform(ctx, tftypes.NewValue(set.ElemType.TerraformType(ctx), tfValue))
elemType := set.ElementType(ctx)
elemValue, err := elemType.ValueFromTerraform(ctx, tftypes.NewValue(elemType.TerraformType(ctx), tfValue))

if err != nil {
return types.Object{Null: true}, diag.Diagnostics{
Expand Down
30 changes: 21 additions & 9 deletions internal/fwserver/attribute_plan_modification.go
View file
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/hashicorp/terraform-plugin-framework/internal/privatestate"
"github.com/hashicorp/terraform-plugin-framework/path"
"github.com/hashicorp/terraform-plugin-framework/tfsdk"
"github.com/hashicorp/terraform-plugin-framework/types"
)

type ModifyAttributePlanResponse struct {
Expand Down Expand Up @@ -85,6 +86,11 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
return
}

// Null and unknown values should not have nested schema to modify.
if req.AttributePlan.IsNull() || req.AttributePlan.IsUnknown() {
return
}

if a.GetAttributes() == nil || len(a.GetAttributes().GetAttributes()) == 0 {
return
}
Expand Down Expand Up @@ -116,7 +122,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
return
}

for idx, planElem := range planList.Elems {
planElements := planList.Elements()

for idx, planElem := range planElements {
attrPath := req.AttributePath.AtListIndex(idx)

configObject, diags := listElemObject(ctx, attrPath, configList, idx, fwschemadata.DataDescriptionConfiguration)
Expand Down Expand Up @@ -193,10 +201,10 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
resp.Private = attrResp.Private
}

planList.Elems[idx] = planObject
planElements[idx] = planObject
}

resp.AttributePlan = planList
resp.AttributePlan = types.ListValue(planList.ElementType(ctx), planElements)
case fwschema.NestingModeSet:
configSet, diags := coerceSetValue(req.AttributePath, req.AttributeConfig)

Expand All @@ -222,7 +230,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
return
}

for idx, planElem := range planSet.Elems {
planElements := planSet.Elements()

for idx, planElem := range planElements {
attrPath := req.AttributePath.AtSetValue(planElem)

configObject, diags := setElemObject(ctx, attrPath, configSet, idx, fwschemadata.DataDescriptionConfiguration)
Expand Down Expand Up @@ -299,10 +309,10 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
resp.Private = attrResp.Private
}

planSet.Elems[idx] = planObject
planElements[idx] = planObject
}

resp.AttributePlan = planSet
resp.AttributePlan = types.SetValue(planSet.ElementType(ctx), planElements)
case fwschema.NestingModeMap:
configMap, diags := coerceMapValue(req.AttributePath, req.AttributeConfig)

Expand All @@ -328,7 +338,9 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
return
}

for key, planElem := range planMap.Elems {
planElements := planMap.Elements()

for key, planElem := range planElements {
attrPath := req.AttributePath.AtMapKey(key)

configObject, diags := mapElemObject(ctx, attrPath, configMap, key, fwschemadata.DataDescriptionConfiguration)
Expand Down Expand Up @@ -405,10 +417,10 @@ func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req tfsdk.Mo
resp.Private = attrResp.Private
}

planMap.Elems[key] = planObject
planElements[key] = planObject
}

resp.AttributePlan = planMap
resp.AttributePlan = types.MapValue(planMap.ElementType(ctx), planElements)
case fwschema.NestingModeSingle:
configObject, diags := coerceObjectValue(req.AttributePath, req.AttributeConfig)

Expand Down
36 changes: 18 additions & 18 deletions internal/fwserver/attribute_plan_modification_test.go
View file
Expand Up @@ -65,7 +65,7 @@ func TestAttributeModifyPlan(t *testing.T) {
AttributeState: types.String{Value: "TESTATTRONE"},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.String{Value: "MODIFIED_TWO"},
AttributePlan: types.StringValue("MODIFIED_TWO"),
Private: testEmptyProviderData,
},
},
Expand Down Expand Up @@ -179,13 +179,13 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.List{
ElemType: types.ObjectType{
AttributePlan: types.ListValue(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
},
},
Elems: []attr.Value{
[]attr.Value{
types.Object{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
Expand All @@ -195,7 +195,7 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
},
},
),
Private: testProviderData,
},
},
Expand Down Expand Up @@ -270,13 +270,13 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.Set{
ElemType: types.ObjectType{
AttributePlan: types.SetValue(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
},
},
Elems: []attr.Value{
[]attr.Value{
types.Object{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
Expand All @@ -286,7 +286,7 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
},
},
),
Private: testProviderData,
},
},
Expand Down Expand Up @@ -401,14 +401,14 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.Set{
ElemType: types.ObjectType{
AttributePlan: types.SetValue(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
"nested_required": types.StringType,
},
},
Elems: []attr.Value{
[]attr.Value{
types.Object{
AttrTypes: map[string]attr.Type{
"nested_computed": types.StringType,
Expand All @@ -430,7 +430,7 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
},
},
),
Private: testEmptyProviderData,
},
},
Expand Down Expand Up @@ -505,13 +505,13 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.Map{
ElemType: types.ObjectType{
AttributePlan: types.MapValue(
types.ObjectType{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
},
},
Elems: map[string]attr.Value{
map[string]attr.Value{
"testkey": types.Object{
AttrTypes: map[string]attr.Type{
"nested_attr": types.StringType,
Expand All @@ -521,7 +521,7 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
},
},
),
Private: testProviderData,
},
},
Expand Down Expand Up @@ -704,7 +704,7 @@ func TestAttributeModifyPlan(t *testing.T) {
},
},
expectedResp: ModifyAttributePlanResponse{
AttributePlan: types.String{Value: "TESTATTRTWO"},
AttributePlan: types.StringValue("TESTATTRTWO"),
RequiresReplace: path.Paths{
path.Root("test"),
},
Expand Down
6 changes: 3 additions & 3 deletions internal/fwserver/attribute_validation.go
View file
Expand Up @@ -153,7 +153,7 @@ func AttributeValidateNestedAttributes(ctx context.Context, a fwschema.Attribute
return
}

for idx := range l.Elems {
for idx := range l.Elements() {
for nestedName, nestedAttr := range a.GetAttributes().GetAttributes() {
nestedAttrReq := tfsdk.ValidateAttributeRequest{
AttributePath: req.AttributePath.AtListIndex(idx).AtName(nestedName),
Expand Down Expand Up @@ -183,7 +183,7 @@ func AttributeValidateNestedAttributes(ctx context.Context, a fwschema.Attribute
return
}

for _, value := range s.Elems {
for _, value := range s.Elements() {
for nestedName, nestedAttr := range a.GetAttributes().GetAttributes() {
nestedAttrReq := tfsdk.ValidateAttributeRequest{
AttributePath: req.AttributePath.AtSetValue(value).AtName(nestedName),
Expand Down Expand Up @@ -213,7 +213,7 @@ func AttributeValidateNestedAttributes(ctx context.Context, a fwschema.Attribute
return
}

for key := range m.Elems {
for key := range m.Elements() {
for nestedName, nestedAttr := range a.GetAttributes().GetAttributes() {
nestedAttrReq := tfsdk.ValidateAttributeRequest{
AttributePath: req.AttributePath.AtMapKey(key).AtName(nestedName),
Expand Down

0 comments on commit 4dfa83c

Please sign in to comment.