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

Define Set type #53

Closed
kmoe opened this issue Jun 23, 2021 · 5 comments · Fixed by #126
Closed

Define Set type #53

kmoe opened this issue Jun 23, 2021 · 5 comments · Fixed by #126
Assignees
Labels
enhancement New feature or request sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Milestone

Comments

@kmoe
Copy link
Member

kmoe commented Jun 23, 2021

Backed by an actual Set implementation.

@kmoe kmoe added the enhancement New feature or request label Jun 23, 2021
@paddycarver paddycarver added this to the v0.3.0 milestone Jul 27, 2021
@paddycarver
Copy link
Contributor

I originally thought we wanted to use a third-party Go implementation for this, but the more I think about it, the more convinced I am that we actually want to build our own, in-house set implementation using attr.Value as the elements of the set.

@bflad
Copy link
Contributor

bflad commented Aug 13, 2021

This might have been developer error on my part, but in trying to implement an "ordered set" type (list with required ordering, but element-uniqueness constraints), I was having trouble with creating a map[tftypes.Value]struct{} due to tftypes.primitive not being hashable (resultant in a Go panic). If that does turn out to be a problem, we might need to figure out the best way to hash values.

Panic:

panic: runtime error: hash of unhashable type tftypes.primitive [recovered]
	panic: runtime error: hash of unhashable type tftypes.primitive

goroutine 14 [running]:
testing.tRunner.func1.2(0x1631d00, 0xc000412090)
	/usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc0003bd800)
	/usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1146 +0x4b6
panic(0x1631d00, 0xc000412090)
	/usr/local/Cellar/go/1.16.6/libexec/src/runtime/panic.go:965 +0x1b9
type..hash.git.luolix.top/hashicorp/terraform-plugin-go/tftypes.Value(0xc0000df168, 0x0, 0x18)
	<autogenerated>:1 +0x35
github.com/hashicorp/terraform-plugin-framework/types.OrderedSetType.Validate(0x0, 0x0, 0x17aee90, 0xc00011a008, 0x17b2428, 0xc0003ab230, 0x15f87a0, 0xc00000e978, 0x169d940, 0x1, ...)
	/Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/types/orderedset.go:47 +0x2a5

Code:

package types

import (
	"context"
	"fmt"
	"strings"

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

var (
	_ attr.TypeWithValidate = OrderedSetType{}
)

type OrderedSetType struct {
	ListType
}

func (t OrderedSetType) Validate(ctx context.Context, in tftypes.Value) []*tfprotov6.Diagnostic {
	var diags []*tfprotov6.Diagnostic

	if !in.Type().Is(tftypes.List{}) {
		err := fmt.Errorf("expected List value, received %T with value: %v", in, in)
		return append(diags, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "OrderedSet Type Validation Error",
			Detail:   "An unexpected error was encountered trying to validate an attribute value. This is always an error in the provider. Please report the following to the provider developer:\n\n" + err.Error(),
		})
	}

	var vals []tftypes.Value

	if err := in.As(&vals); err != nil {
		return append(diags, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "OrderedSet Type Validation Error",
			Detail:   "An unexpected error was encountered trying to validate an attribute value. This is always an error in the provider. Please report the following to the provider developer:\n\n" + err.Error(),
		})
	}

	duplicatesMap := make(map[tftypes.Value]struct{})
	valsMap := make(map[tftypes.Value]struct{})

	for _, val := range vals {
		if _, ok := valsMap[val]; ok {
			if _, ok := duplicatesMap[val]; ok {
				continue
			}

			duplicatesMap[val] = struct{}{}
			continue
		}
		valsMap[val] = struct{}{}
	}

	if len(duplicatesMap) > 0 {
		var duplicates []string

		for duplicate := range duplicatesMap {
			duplicates = append(duplicates, duplicate.String())
		}

		return append(diags, &tfprotov6.Diagnostic{
			Severity: tfprotov6.DiagnosticSeverityError,
			Summary:  "Duplicate Set Elements",
			Detail:   fmt.Sprintf("This attribute contains duplicate elements of:\n\n%s", strings.Join(duplicates, "\n")),
		})
	}

	return nil
}

@paddycarver
Copy link
Contributor

I think my hot take is that we should start with unordered sets, as that's what the Terraform type expects, and right now there's no way to surface a tftypes.Set that is expected to be unordered.

The string representation of the value should be consistent, but I think what we really want is to use the Equal method instead of relying on Go's type system's concept of equality, to avoid any gotchas in the future around that.

@bflad
Copy link
Contributor

bflad commented Aug 16, 2021

Certainly, opened #99 to track the potential for an "ordered set" type, unordered sets (surfacing tftypes.Set) is a much more important goal. I only documented the above since I think it means we'll need to create our own hashing (or introduce more complex algorithms to work with the data like a set) unless we adjust upstream.

@paddycarver paddycarver modified the milestones: v0.3.0, v0.4.0 Aug 30, 2021
@bflad bflad self-assigned this Sep 1, 2021
@paddycarver paddycarver added the sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it. label Sep 9, 2021
bflad added a commit that referenced this issue Sep 14, 2021
@github-actions
Copy link

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 Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants