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

Cannot write state to interface in generic func #1035

Open
JacobPotter opened this issue Sep 12, 2024 · 9 comments
Open

Cannot write state to interface in generic func #1035

JacobPotter opened this issue Sep 12, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@JacobPotter
Copy link

Module version

github.com/hashicorp/terraform-plugin-framework v1.11.0

Relevant provider source code

SEE ACTUAL BEHAVIOR FOR EXAMPLES

Terraform Configuration Files

...

Debug Output

Expected Behavior

Actual Behavior

We are using an interface for our resource models that stores the state of a provider resource. This interface defines methods for converting data from API to TF and vice versa. For example:

We have an interface here:

type ResourceTransform[M any] interface {
	GetApiModelFromTfModel(context.Context) (M, diag.Diagnostics)
	GetTfModelFromApiModel(context.Context, M) diag.Diagnostics
}
type ResourceTransformWithID[M any] interface {
	GetID() int64
	ResourceTransform[M]
}

Which is implemented by the model below

type DynamicContentVariantModel struct {
	ID       types.Int64  `tfsdk:"id"`
	Content  types.String `tfsdk:"content"`
	LocaleID types.Int64  `tfsdk:"locale_id"`
	Active   types.Bool   `tfsdk:"active"`
	Default  types.Bool   `tfsdk:"default"`
}

type DynamicContentItemResourceModel struct {
	ID              types.Int64                  `tfsdk:"id"`
	Name            types.String                 `tfsdk:"name"`
	Placeholder     types.String                 `tfsdk:"placeholder"`
	DefaultLocaleID types.Int64                  `tfsdk:"default_locale_id"`
	Variants        []DynamicContentVariantModel `tfsdk:"variants"`
}

func (d *DynamicContentItemResourceModel) GetID() int64 {
	...
}

func (d *DynamicContentItemResourceModel) GetApiModelFromTfModel(_ context.Context) (dci zendesk.DynamicContentItem, diags diag.Diagnostics) {
      ...
}

func (d *DynamicContentItemResourceModel) GetTfModelFromApiModel(_ context.Context, dci zendesk.DynamicContentItem) (diags diag.Diagnostics) {
	...
}

If I try to use a generic function like this:

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
	response.Diagnostics.Append(request.Plan.Get(ctx, &resourceModel)...)

	if response.Diagnostics.HasError() {
		return
	}
	newResource, diags := resourceModel.GetApiModelFromTfModel(ctx)

	response.Diagnostics.Append(diags...)

	if response.Diagnostics.HasError() {
		return
	}

	resp, err := createFunc(ctx, newResource)

	if err != nil {
		response.Diagnostics.AddError("Error creating resource", fmt.Sprintf("Error: %s", err))
		return
	}

	response.Diagnostics.Append(resourceModel.GetTfModelFromApiModel(ctx, resp)...)

	if response.Diagnostics.HasError() {
		return
	}

	response.Diagnostics.Append(response.State.Set(ctx, resourceModel)...)
}

I get the following error when trying to apply:

 Error: Value Conversion Error
        
          with zendesk_dynamic_content.test,
        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:
        
        don't know how to reflect tftypes.Object["default_locale_id":tftypes.Number,
        "id":tftypes.Number, "name":tftypes.String, "placeholder":tftypes.String,
        "variants":tftypes.List[tftypes.Object["active":tftypes.Bool,
        "content":tftypes.String, "default":tftypes.Bool, "id":tftypes.Number,
        "locale_id":tftypes.Number]]] into
        models.ResourceTransformWithID[github.com/JacobPotter/go-zendesk/zendesk.DynamicContentItem]

It looks like request.Plan.Get is having an issue writing the state data into the model when using an interface.

Steps to Reproduce

  1. Write terraform plan/state data into model via interface

References

@JacobPotter JacobPotter added the bug Something isn't working label Sep 12, 2024
@SBGoods
Copy link
Contributor

SBGoods commented Sep 12, 2024

Hi @JacobPotter 👋🏾,

Sorry that you're running into trouble here. The (request).Plan.Get() function uses reflection to build the plan into the target type. We intentionally don't support interfaces as a target type, so this would be a new feature request rather than a bug. To set expectations, I'm not entirely sure if we can support interfaces in the reflection logic without digging deeper.

As a workaround, you can try implementing the tftypes.ValueConverter interface on ResourceTransform[M], which should allow the value to be built without using reflection.

@JacobPotter
Copy link
Author

JacobPotter commented Sep 27, 2024

@SBGoods Can you show an example of how ValueConverter would be implemented?

EDIT: I am trying to implement it, but I am unsure how the ValueConverter method would interact with the model and what it would return

@JacobPotter
Copy link
Author

@SBGoods I have updated my comment above, bumping for visibility.

@austinvalle
Copy link
Member

Hey there @JacobPotter, unfortunately there aren't a ton of examples of implementing that interface since most provider implementations just use the reflection as-is.

There is a dynamic resource that Terraform core uses for it's own testing that implements it: https://github.com/hashicorp/terraform-provider-tfcoremock/blob/b8029786d035fdc5d5003511b3299da3a17ef74e/internal/data/resource.go#L67

It also implements the tftypes.ValueCreator interface, which you'll also need if you're attempting to use (tfsdk.State).Set: https://github.com/hashicorp/terraform-provider-tfcoremock/blob/b8029786d035fdc5d5003511b3299da3a17ef74e/internal/data/resource.go#L60

@JacobPotter
Copy link
Author

@austinvalle

So I attempted to implement both interfaces, and I am running into an issue with reflect.

Here is my implementation of FromTerraform5Value

func (g *GroupResourceModel) FromTerraform5Value(value tftypes.Value) error {

	// It has to be an object we are converting from.
	if !value.Type().Is(tftypes.Object{}) {
		return errors.New("can only convert between object types")
	}

	values, err := fromTerraform5Value(value)
	if err != nil {
		return err
	}

	// We know these kinds of conversions are safe now, as we checked the type
	// at the beginning.

	if v, ok := values.(map[string]any); ok {
		switch v["id"].(type) {
		case big.Float, *big.Float:
			i, _ := v["id"].(*big.Float).Int64()
			g.ID = types.Int64Value(i)
		case int, int64:
			g.ID = types.Int64Value(v["id"].(int64))
		default:
			return fmt.Errorf("bad id value type: %v", reflect.TypeOf(v))
		}

		g.URL = types.StringValue(v["url"].(string))
		g.Name = types.StringValue(v["name"].(string))
		g.Default = types.BoolValue(v["default"].(bool))
		g.Deleted = types.BoolValue(v["deleted"].(bool))
		g.IsPublic = types.BoolValue(v["is_public"].(bool))
		g.Description = types.StringValue(v["description"].(string))
		g.CreatedAt = types.StringValue(v["created_at"].(string))
		g.UpdatedAt = types.StringValue(v["updated_at"].(string))

	} else {
		return errors.New("can only convert between object types")
	}

	return nil
}

When trying to run, am getting a panic:


goroutine 158 [running]:
reflect.Value.Method({0x10107a1a0, 0x1018d82a0, 0x94}, 0x0)
   /Users/jacob.potter/sdk/go1.22.5/src/reflect/value.go:2082 +0x19c
reflect.Value.MethodByName({0x10107a1a0, 0x1018d82a0, 0x94}, {0x100de841a, 0x13})
   /Users/jacob.potter/sdk/go1.22.5/src/reflect/value.go:2121 +0x174
github.com/hashicorp/terraform-plugin-framework/internal/reflect.NewValueConverter({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x10107a1a0, 0x140006a6930, ...}, ...)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/reflect/interfaces.go:296 +0xac
github.com/hashicorp/terraform-plugin-framework/internal/reflect.BuildValue({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x10107a1a0, 0x140006a6930, ...}, ...)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/reflect/into.go:77 +0x2d0
github.com/hashicorp/terraform-plugin-framework/internal/reflect.Into({0x10119dd90, 0x140006a4390}, {0x10119fed0, 0x140006a5ef0}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x100fb5760, 0x140006a6930}, ...)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/reflect/into.go:42 +0x358
github.com/hashicorp/terraform-plugin-framework/internal/fwschemadata.Data.Get({{0x100dd92e1, 0x4}, {0x1011a4608, 0x14000032d20}, {{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}}, {0x10119dd90, 0x140006a4390}, ...)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/fwschemadata/data_get.go:16 +0xc8
github.com/hashicorp/terraform-plugin-framework/tfsdk.Plan.Get({{{0x1011a28a0, 0x140006a5620}, {0x101023740, 0x140006a5530}}, {0x1011a4608, 0x14000032d20}}, {0x10119dd90, 0x140006a4390}, {0x100fb5760, 0x140006a6930})
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/tfsdk/plan.go:24 +0xc4
github.com/Dynatrace/terraform-provider-zendesk/internal/provider/models.CreateResource[...]({0x10119dd90, 0x140006a4390}, {{{{0x1011a28a0, 0x140006a4d80}, {0x101023740, 0x140006a4c90}}, {0x1011a4608, 0x14000032d20}}, {{{0x1011a28a0, 0x140006a5620}, ...}, ...}, ...}, ...)
   /Users/jacob.potter/Bitbucket/terraform-provider-zendesk/internal/provider/models/shared.go:30 +0xd0
github.com/Dynatrace/terraform-provider-zendesk/internal/provider.(*GroupResource).Create(0x140004ac0f8, {0x10119dd90, 0x140006a4390}, {{{{0x1011a28a0, 0x140006a4d80}, {0x101023740, 0x140006a4c90}}, {0x1011a4608, 0x14000032d20}}, {{{0x1011a28a0, ...}, ...}, ...}, ...}, ...)
   /Users/jacob.potter/Bitbucket/terraform-provider-zendesk/internal/provider/group_resource.go:53 +0x128
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).CreateResource(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x140006c4760, 0x140006c4730)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/fwserver/server_createresource.go:101 +0x514
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).ApplyResourceChange(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x14000622910, 0x140006c4980)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/fwserver/server_applyresourcechange.go:57 +0x2d8
github.com/hashicorp/terraform-plugin-framework/internal/proto6server.(*Server).ApplyResourceChange(0x140004bc000, {0x10119dd90, 0x140006a4390}, 0x14000622870)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/internal/proto6server/server_applyresourcechange.go:55 +0x538
github.com/hashicorp/terraform-plugin-go/tfprotov6/tf6server.(*server).ApplyResourceChange(0x140004da280, {0x10119dd90, 0x140006a4240}, 0x140004d23f0)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov6/tf6server/server.go:865 +0x3cc
github.com/hashicorp/terraform-plugin-go/tfprotov6/internal/tfplugin6._Provider_ApplyResourceChange_Handler({0x10115ca40, 0x140004da280}, {0x10119dd90, 0x14000323530}, 0x14000511280, 0x0)
   /Users/jacob.potter/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov6/internal/tfplugin6/tfplugin6_grpc.pb.go:545 +0x2a4
google.golang.org/grpc.(*Server).processUnaryRPC(0x140000f6400, {0x10119dd90, 0x140003234a0}, {0x1011a3440, 0x140001371e0}, 0x14000316fc0, 0x14000322d50, 0x10185cdd8, 0x0)
   /Users/jacob.potter/go/pkg/mod/google.golang.org/[email protected]/server.go:1394 +0x1480
google.golang.org/grpc.(*Server).handleStream(0x140000f6400, {0x1011a3440, 0x140001371e0}, 0x14000316fc0)
   /Users/jacob.potter/go/pkg/mod/google.golang.org/[email protected]/server.go:1805 +0xd0c
google.golang.org/grpc.(*Server).serveStreams.func2.1()
   /Users/jacob.potter/go/pkg/mod/google.golang.org/[email protected]/server.go:1029 +0x144
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 184
   /Users/jacob.potter/go/pkg/mod/google.golang.org/[email protected]/server.go:1040 +0x1c8

Looking at a debugger when this happens, it seems to be failing when trying to create a NewValueConverter:
image

So my implementation of FromTerraform5Value does not even appear to be run; it is getting caught up in making sure it exists I think.

@austinvalle
Copy link
Member

Hmm, very interesting! That looks like there's a potential missing case in our reflection utility before trying to retrieve that method. A lot of the reflection logic was written pre-generics, so it wouldn't be surprising 🤔. I'll try to recreate that just in the framework logic and report back.

@JacobPotter
Copy link
Author

Yeah, it would be nice if there was cleaner support for generics in general. Needing to implement ValueCreator and ValueConverter almost brings back the overhead that was reduced by using generics in the first place. I'm just trying to avoid the repetitive code that we currently have in our provider.

@austinvalle
Copy link
Member

Just spent some time looking at your examples and I think I may have skipped by a detail in there:

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
	response.Diagnostics.Append(request.Plan.Get(ctx, &resourceModel)...)

In your provider, for parameter resourceModel, I'm assuming this is populated by a struct pointer? Like of type *GroupResourceModel?

If that's the case I think you'd want to pass that in directly, rather than passing a pointer of resourceModel which I believe would be the interface (ResourceTransformWithID[M]) with a nil value?

func CreateResource[M any](ctx context.Context, request resource.CreateRequest, response *resource.CreateResponse, resourceModel ResourceTransformWithID[M], createFunc func(ctx context.Context, newResource M) (M, error)) {
	// resourceModel is already a pointer to a struct
	response.Diagnostics.Append(request.Plan.Get(ctx, resourceModel)...)

If you're able to do that, I don't believe you'll need to implement ValueConverter because you're passing our reflection logic a pointer to the struct which has the correct tfsdk tags on it.


Side note: I notice your provider is private, but are you able to share some of the code that instantiates/calls your CreateResource[M any] method? That'd make it a little easier for me to help debug further and make sure we're on the same page.

@austinvalle
Copy link
Member

austinvalle commented Oct 9, 2024

Not related to my comment above, but semi-related to the overall conversation

While attempting to recreate the behavior, I think I stumbled across a different bug, top-level structs that implement tftypes.ValueConverter with pointer receiver methods aren't being detected properly because our reflection logic grabs the element of the pointer during the Into function, which is our entry point underneath request.Plan.Get. If you implement tftypes.ValueConverter with value receiver methods then it'd work fine, although that's not how they are typically implemented 🙃

Fields on a struct that implement tftypes. ValueConverter would still work however, which seems to be the majority of test cases, so likely just an oversight we could fix with some refactoring.


With that aside, I'm still thinking we should be able to resolve your problem by passing in the variable that contains the pointer struct (resourceModel => *GroupResourceModel) rather than the pointer of the generic interface (&resourceModel => ResourceTransformWithID[M](nil))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants