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

Panic for unset types.Number #89

Closed
keldin-coding opened this issue Jul 29, 2021 · 3 comments · Fixed by #200
Closed

Panic for unset types.Number #89

keldin-coding opened this issue Jul 29, 2021 · 3 comments · Fixed by #200
Assignees
Labels
bug Something isn't working reflection Issues and PRs about the reflection subsystem used to convert between attr.Values and Go values. types Issues and pull requests about our types abstraction and implementations.
Milestone

Comments

@keldin-coding
Copy link

keldin-coding commented Jul 29, 2021

Module version

module terraform-provider-clubhouse

go 1.16

require (
	github.com/hashicorp/terraform-plugin-framework v0.2.0
	github.com/hashicorp/terraform-plugin-go v0.3.1
	golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4 // indirect
	golang.org/x/sys v0.0.0-20210510120138-977fb7262007 // indirect
	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
)

Relevant provider source code

https://gist.github.com/lirossarvet/8be0ce992f2ac44edf60ff28d31681b0

This is the relevant chunk, I think, or at least enough. The whole resource is in the gist, though.

package clubhouse

import (
	"context"
	"fmt"
	"terraform-provider-clubhouse/client"

	// "github.com/hashicorp-demoapp/hashicups-client-go"
	"github.com/hashicorp/terraform-plugin-framework/schema"
	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
	"github.com/hashicorp/terraform-plugin-framework/types"

	// "github.com/hashicorp/terraform-plugin-framework/types"
	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
)

type resourceStoryType struct{}

// Order Resource schema
func (r resourceStoryType) GetSchema(_ context.Context) (schema.Schema, []*tfprotov6.Diagnostic) {
	return schema.Schema{
		Attributes: map[string]schema.Attribute{
			"name": {
				Type:     types.StringType,
				Required: true,
			},
			"project_id": {
				Type:     types.NumberType,
				Required: true,
			},
			"description": {
				Type:     types.StringType,
				Required: true,
			},
			"epic_id": {
				Type:     types.NumberType,
				Optional: true,
			},
			"story_type": {
				Type:     types.StringType,
				Optional: true,
				Computed: true,
			},
			"group_id": {
				Type:     types.StringType,
				Optional: true,
			},
			"id": {
				Type:     types.NumberType,
				Computed: true,
			},
		},
	}, nil
}

// New resource instance
func (r resourceStoryType) NewResource(_ context.Context, p tfsdk.Provider) (tfsdk.Resource, []*tfprotov6.Diagnostic) {
	return resourceStory{
		p: *(p.(*provider)),
	}, nil
}

type resourceStory struct {
	p provider
}

type resourceStoryData struct {
	Name        types.String `tfsdk:"name"`
	ProjectId   types.Number `tfsdk:"project_id"`
	Description types.String `tfsdk:"description"`
	EpicId      types.Number `tfsdk:"epic_id"`
	Id          types.Number `tfsdk:"id"`
	GroupId     types.String `tfsdk:"group_id"`
	StoryType   types.String `tfsdk:"story_type"`
}

// Create a new resource
func (r resourceStory) Create(ctx context.Context, req tfsdk.CreateResourceRequest, resp *tfsdk.CreateResourceResponse) {
	if !r.p.configured {
		resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "Provider not configured",
			Detail:   "The provider hasn't been configured before apply, likely because it depends on an unknown value from another resource. This leads to weird stuff happening, so we'd prefer if you didn't do that. Thanks!",
		})
		return
	}

	var plan resourceStoryData
	err := req.Plan.Get(ctx, &plan)
	if err != nil {
		resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "Error reading plan",
			Detail:   "An unexpected error was encountered while reading the plan: " + err.Error(),
		})
		return
	}

	rawStory := client.Story{
		Name:        plan.Name.Value,
		ProjectId:   convertBigFloatToInt64(plan.ProjectId.Value),
		Description: plan.Description.Value,
	}

	if !plan.StoryType.Unknown && !plan.StoryType.Null {
		rawStory.StoryType = &(plan.StoryType.Value)
	}

	if !plan.EpicId.Null && !plan.EpicId.Unknown {
		i := convertBigFloatToInt64(plan.EpicId.Value)
		rawStory.EpicId = &i
	}

	if !plan.GroupId.Null {
		rawStory.GroupId = &plan.GroupId.Value
	}

	created, httpErr := r.p.client.CreateStory(ctx, rawStory)
	if httpErr != nil {
		resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "Failed to create",
			Detail:   "An unexpected error was encountered while creating the resource: " + httpErr.Error(),
		})
		return
	}

	forState := resourceStoryData{
		Id:          types.Number{Value: convertInt64ToBigFloat(*created.Id)},
		ProjectId:   types.Number{Value: convertInt64ToBigFloat(created.ProjectId)},
		Name:        types.String{Value: created.Name},
		Description: types.String{Value: created.Description},
	}

	if created.StoryType != nil {
		forState.StoryType = types.String{Value: *created.StoryType}
	}

	if created.EpicId != nil {
		forState.EpicId = types.Number{Value: convertInt64ToBigFloat(*created.EpicId)}
	} else {
		forState.EpicId = types.Number{Null: true}
	}

	if created.GroupId != nil {
		forState.GroupId = types.String{Value: *created.GroupId}
	}

	resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{
		Severity: tfprotov6.DiagnosticSeverityWarning,
		Summary:  "What're we doing",
		Detail:   fmt.Sprintf("HERE WE GO\nCreatead: %+v\nForState %+v\n", created, forState),
	})
	return

	stateError := resp.State.Set(ctx, forState)
	if stateError != nil {
		resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "Failed to set state",
			Detail:   "An unexpected error was encountered while setting the state: " + stateError.Error(),
		})
		return
	}
}

API Client struct

type Story struct {
	Name        string `json:"name"`
	ProjectId   int64  `json:"project_id"`
	Description string `json:"description"`

	EpicId    *int64  `json:"epic_id,omitempty"`
	Id        *int64  `json:"id,omitempty"`
	StoryType *string `json:"story_type,omitempty"`
	GroupId   *string `json:"group_id"`
	Archived  *bool   `json:"archived,omitempty"`
}

Terraform Configuration Files

terraform {
  required_providers {
    clubhouse = {
      version = ">= 0.0.1"
      source  = "github.com/firehydrant/clubhouse"
    }
  }
  required_version = "~> 1.0.3"
}

provider "clubhouse" {}

// Notice that epic_id, an Optional Attribute, is unset.
resource "clubhouse_story" "safety" {
  name        = "Hooty Hoo"
  project_id  = 1 // Not a real ID
  group_id    = "aca6a0e5-690e-4b5c-9fbd-dc73f8109a9d" // not a real ID in this case
  description = <<-EOT
  HOO HOO
  EOT
}

Debug Output

Panic output is in the gist posted above. I can share the trace logs if needed.

Expected Behavior

An unset Optional types.Number attribute would not cause a panic and would instead not set the value in state or ignore it.

Actual Behavior

Panic! (in the terraform provider)

Steps to Reproduce

  1. Create a Terraform Provider using the framework
  2. Have an attribute of type types.NumberType and Optional: true
  3. Construct the struct to pass into State.Set with something like:
type MyResourceData struct {
  MyAttribute types.Number `tfsdk:my_attribute"
}

// in Create method
m := MyResourceData

err := resp.State.Set(ctx, &m)

References

@keldin-coding keldin-coding added the bug Something isn't working label Jul 29, 2021
@paddycarver paddycarver added types Issues and pull requests about our types abstraction and implementations. reflection Issues and PRs about the reflection subsystem used to convert between attr.Values and Go values. labels Sep 21, 2021
@bflad bflad self-assigned this Oct 5, 2021
@bflad
Copy link
Contributor

bflad commented Oct 5, 2021

Hi @lirossarvet 👋 Thank you so much for filing this detailed bug report. This is certainly not expected behavior! We have a fix for this that will be proposed soon and we will let you know when this is resolved.

In the meantime, using *big.Float directly (or another number pointer type, such as *int64) should allow this use case to work as expected. e.g.

type MyResourceData struct {
  MyAttribute *big.Float `tfsdk:my_attribute"
}

bflad added a commit to hashicorp/terraform-plugin-go that referenced this issue Oct 5, 2021
Reference: hashicorp/terraform-plugin-framework#89

This matches the consistency of other pointer types in the `valueFromNumber` function and can prevent unexpected panics.
bflad added a commit to hashicorp/terraform-plugin-go that referenced this issue Oct 5, 2021
…114)

Reference: hashicorp/terraform-plugin-framework#89

This matches the consistency of other pointer types in the `valueFromNumber` function and can prevent unexpected panics.
bflad added a commit that referenced this issue Oct 5, 2021
Reference: #89
Reference: hashicorp/terraform-plugin-go#114

This change will ensure that if the `Number` `Value` pointer is `nil`, that the returned value is an untyped `nil`, rather than a `*big.Float` typed `nil`.

The upstream `(tftypes.Value).IsNull()` method can only detect untyped `nil` for values. A related fix for `tftypes.NewValue()` to convert a `*big.Float` `nil` value into an untyped `nil` has also been merged upstream to fix this in depth.

Previously, this new test case would pass:

```go
		"value-nil": {
			input:       Number{Value: nil},
			expectation: (*big.Float)(nil),
		},
```

Now it appropriately expects:

```go
		"value-nil": {
			input:       Number{Value: nil},
			expectation: nil,
		},
```
@bflad bflad closed this as completed in #200 Oct 5, 2021
bflad added a commit that referenced this issue Oct 5, 2021
Reference: #89
Reference: hashicorp/terraform-plugin-go#114

This change will ensure that if the `Number` `Value` pointer is `nil`, that the returned value is an untyped `nil`, rather than a `*big.Float` typed `nil`.

The upstream `(tftypes.Value).IsNull()` method can only detect untyped `nil` for values. A related fix for `tftypes.NewValue()` to convert a `*big.Float` `nil` value into an untyped `nil` has also been merged upstream to fix this in depth.

Previously, this new test case would pass:

```go
		"value-nil": {
			input:       Number{Value: nil},
			expectation: (*big.Float)(nil),
		},
```

Now it appropriately expects:

```go
		"value-nil": {
			input:       Number{Value: nil},
			expectation: nil,
		},
```
@bflad bflad added this to the v0.5.0 milestone Oct 5, 2021
@bflad
Copy link
Contributor

bflad commented Oct 5, 2021

The fix for this has been merged and will be part of the next terraform-plugin-framework release, either v0.4.3 (if shipped) or v0.5.0, in the coming weeks. 🎉

@github-actions
Copy link

github-actions bot commented Nov 5, 2021

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 github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working reflection Issues and PRs about the reflection subsystem used to convert between attr.Values and Go values. types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants