terraform-plugin-framework icon indicating copy to clipboard operation
terraform-plugin-framework copied to clipboard

Unset Primitive Types on Optional Attributes with (State).Set() Generate Errors

Open bflad opened this issue 4 years ago • 1 comments

Module version

v0.4.2

Relevant provider source code

package provider

import (
	"context"

	"github.com/hashicorp/terraform-plugin-framework/diag"
	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
	"github.com/hashicorp/terraform-plugin-framework/types"
)

type optionalTypesResourceType struct{}

func (t optionalTypesResourceType) GetSchema(ctx context.Context) (tfsdk.Schema, diag.Diagnostics) {
	return tfsdk.Schema{
		Attributes: map[string]tfsdk.Attribute{
			"id": {
				Type:     types.StringType,
				Computed: true,
			},
			"optional_types_bool": {
				Type:     types.BoolType,
				Optional: true,
			},
			"optional_types_float64": {
				Type:     types.Float64Type,
				Optional: true,
			},
			"optional_types_int64": {
				Type:     types.Int64Type,
				Optional: true,
			},
			"optional_types_string": {
				Type:     types.StringType,
				Optional: true,
			},
		},
	}, nil
}

func (t optionalTypesResourceType) NewResource(ctx context.Context, p tfsdk.Provider) (tfsdk.Resource, diag.Diagnostics) {
	return optionalTypesResource{}, nil
}

type optionalTypesResourceTypeData struct {
	ID                   string        `tfsdk:"id"`
	OptionalTypesBool    types.Bool    `tfsdk:"optional_types_bool"`
	OptionalTypesFloat64 types.Float64 `tfsdk:"optional_types_float64"`
	OptionalTypesInt64   types.Int64   `tfsdk:"optional_types_int64"`
	OptionalTypesString  types.String  `tfsdk:"optional_types_string"`
}

type optionalTypesResource struct{}

func (r optionalTypesResource) Create(ctx context.Context, req tfsdk.CreateResourceRequest, resp *tfsdk.CreateResourceResponse) {
	data := optionalTypesResourceTypeData{
		ID: "testing",
	}
	resp.Diagnostics.Append(resp.State.Set(ctx, data)...)
}

func (r optionalTypesResource) Read(ctx context.Context, req tfsdk.ReadResourceRequest, resp *tfsdk.ReadResourceResponse) {
	// Intentionally blank for testing.
}

func (r optionalTypesResource) Update(ctx context.Context, req tfsdk.UpdateResourceRequest, resp *tfsdk.UpdateResourceResponse) {
	// Intentionally blank for testing.
}

func (r optionalTypesResource) Delete(ctx context.Context, req tfsdk.DeleteResourceRequest, resp *tfsdk.DeleteResourceResponse) {
	// Intentionally blank for testing.
}

func (r optionalTypesResource) ImportState(ctx context.Context, req tfsdk.ImportResourceStateRequest, resp *tfsdk.ImportResourceStateResponse) {
	tfsdk.ResourceImportStateNotImplemented(ctx, "intentionally not implementated", resp)
}

Terraform Configuration Files

terraform {
  required_providers {
    framework = {
      source = "bflad/framework"
    }
  }
  required_version = "1.0.8"
}

resource "framework_optional_types" "example" {}

Expected Behavior

Either framework generated errors with suggested fixes for the situation, documentation surrounding this problematic use case, or a successful apply with null values in the Terraform state for these optional attributes.

Actual Behavior

Terraform CLI returns error diagnostics:

$ terraform apply
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # framework_optional_types.example will be created
  + resource "framework_optional_types" "example" {
      + id = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.
framework_optional_types.example: Creating...
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to framework_optional_types.example, provider "provider[\"registry.terraform.io/bflad/framework\"]" produced an unexpected new value: .optional_types_bool:
│ was null, but now cty.False.
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
╵
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to framework_optional_types.example, provider "provider[\"registry.terraform.io/bflad/framework\"]" produced an unexpected new value: .optional_types_float64:
│ was null, but now cty.NumberIntVal(0).
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
╵
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to framework_optional_types.example, provider "provider[\"registry.terraform.io/bflad/framework\"]" produced an unexpected new value: .optional_types_int64:
│ was null, but now cty.NumberIntVal(0).
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
╵
╷
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to framework_optional_types.example, provider "provider[\"registry.terraform.io/bflad/framework\"]" produced an unexpected new value: .optional_types_string:
│ was null, but now cty.StringVal("").
│ 
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
╵

Steps to Reproduce

  1. terraform apply

References

  • Related: #89 (although types.Number is now fixed because its zero-value is a nil pointer, which is then set to a null value)

bflad avatar Oct 05 '21 20:10 bflad

This is not an exhaustive list of proposals -- just capturing some initial thoughts.

Proposal 1: Change type implementations to pointer types for Value

Theoretically, the implementations could introduce the following breaking changes so zero-value initialization can be detected:

type Bool struct {
	// Unknown will be true if the value is not yet known.
	Unknown bool

	// Value contains the value, as long as Unknown is false.
	Value *bool
}

type Float64 struct {
	// Unknown will be true if the value is not yet known.
	Unknown bool

	// Value contains the value, as long as Unknown is false.
	Value *float64
}

type Int64 struct {
	// Unknown will be true if the value is not yet known.
	Unknown bool

	// Value contains the value, as long as Unknown is false.
	Value *int64
}

type String struct {
	// Unknown will be true if the value is not yet known.
	Unknown bool

	// Value contains the value, as long as Unknown is false.
	Value *string
}

This would resolve the zero-value issue, as the Value fields would be nil, however, this means that these types differ from other types implementations, which do have a Null field. A workaround for this particular quirk is recommending or requiring implementations to use a proposed (attr.Value).IsNull() method per #193.

This, however, does make working with the implementation much more difficult. For example, creating inline values is not directly possible in Go. e.g. something like this cannot be done

// This example includes invalid Go code
types.String{Value: &"you cannot create a pointer of string literal"}

This could be alleviated if the framework provided pointer conversion functions, e.g.

func strPointer(str string) *string {
  return &str
}

Or types creation functions were introduced:

// In types package
func NewString(str string) types.String {
  return types.String{
    Value: &str,
  }
}

Proposal 2: Allow and Recommend Pointers to Type Implementations

Something that doesn't work today is changing the target type implementation to use pointers to the types types instead:

type optionalTypesResourceTypeData struct {
	ID                   string         `tfsdk:"id"`
	OptionalTypesBool    *types.Bool    `tfsdk:"optional_types_bool"`
	OptionalTypesFloat64 *types.Float64 `tfsdk:"optional_types_float64"`
	OptionalTypesInt64   *types.Int64   `tfsdk:"optional_types_int64"`
	OptionalTypesString  *types.String  `tfsdk:"optional_types_string"`
}

Currently this generates the following panic:

panic: value method github.com/hashicorp/terraform-plugin-framework/types.Bool.ToTerraformValue called using nil *Bool pointer

goroutine 66 [running]:
github.com/hashicorp/terraform-plugin-framework/types.(*Bool).ToTerraformValue(0x0, {0x1697bb8, 0xc000482280})
        <autogenerated>:1 +0x6e
github.com/hashicorp/terraform-plugin-framework/internal/reflect.FromAttributeValue({0x1697bb8, 0xc000482280}, {0x169c3d8, 0x1a06830}, {0x7355200, 0x0}, 0xc0004864b0)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/reflect/interfaces.go:316 +0xe9
github.com/hashicorp/terraform-plugin-framework/internal/reflect.FromValue({0x1697bb8, 0xc000482280}, {0x169c3d8, 0x1a06830}, {0x1545900, 0x0}, 0xc0004864b0)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/reflect/outof.go:22 +0xc45
github.com/hashicorp/terraform-plugin-framework/internal/reflect.FromStruct({0x1697bb8, 0xc000482280}, {0x169f4b0, 0xc0004ca1e0}, {0x1594820, 0xc0004ca1b0, 0x110001530f80}, 0xc0004863f0)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/reflect/struct.go:176 +0x50a
github.com/hashicorp/terraform-plugin-framework/internal/reflect.FromValue({0x1697bb8, 0xc000482280}, {0x169c398, 0xc0004ca1e0}, {0x1594820, 0xc0004ca1b0}, 0xc0004863f0)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/reflect/outof.go:53 +0xab6
github.com/hashicorp/terraform-plugin-framework/tfsdk.(*State).Set(0xc0004b6580, {0x1697bb8, 0xc000482280}, {0x1594820, 0xc0004ca1b0})
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/tfsdk/state.go:98 +0x13d
github.com/bflad/terraform-provider-framework/internal/provider.optionalTypesResource.Create({}, {_, _}, {{{{0x169f5a0, 0xc000488cf0}, {0x1531100, 0xc0004888a0}}, {0xc000488510, 0x0, {0x0, ...}, ...}}, ...}, ...)
        /Users/bflad/test/terraform-provider-framework/internal/provider/resource_optional_types.go:58 +0x94

This would alleviate the zero-value problem using the existing types since the values themselves are pointers and therefore remain unset unless set. The expectation in this case would be that nil values for these pointers would convert to null values of the types.

Proposal 3: Return Framework Diagnostic for Zero-Value Versus Null Differences

Theoretically, the framework can capture the case where the planned value is null and the provider response is a zero-value for the type, then offer suggestions about what happened and how to fix it. This may desirable even with Proposal 1 or 2, to further help provider developers in this scenario.

Proposal 4: Document Error and Fixes at https://www.terraform.io/docs/plugin/framework/writing-state.html

Without changing the code, this class of Provider produced inconsistent result after apply error can be documented on the website and it can offer suggested things to check or fix.

bflad avatar Oct 05 '21 20:10 bflad

I think the latest proposal of switching the zero-value of the framework-defined types to null instead of (generally) a zero-value of the value is the best option here. That proposal is captured in #447 and I'm going to close this issue in preference of that issue since the other proposals captured here feel less than ideal.

bflad avatar Sep 27 '22 12:09 bflad

I am trying to set one of my schema Attribute to Int Pointer, Need some input how could i go about it ? Wondering would something like this would work as getting nil value is case of no inputs.

"interval_month": {
							Type:         schema.ValueType(types.IsNumeric),
							Optional:     true,
							Description:  "Specifies the rotation time interval in months for the instance.",
							ValidateFunc: validate.ValidateAllowedRangeInt(1, 12),
						},

harshit777 avatar Oct 26 '22 08:10 harshit777

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.

github-actions[bot] avatar Nov 26 '22 02:11 github-actions[bot]