all: Add support for embedded struct types in object conversions

.changes/unreleased/ENHANCEMENTS-20240722-175116.yaml

@@ -0,0 +1,6 @@
+kind: ENHANCEMENTS
+body: 'all: Added embedded struct support for object to struct conversions with `tfsdk`
+  tags'
+time: 2024-07-22T17:51:16.833264-04:00
+custom:
+  Issue: "1021"

internal/reflect/helpers.go

@@ -46,35 +46,82 @@ func commaSeparatedString(in []string) string {
 }
 
 // getStructTags returns a map of Terraform field names to their position in
-// the tags of the struct `in`. `in` must be a struct.
-func getStructTags(_ context.Context, in reflect.Value, path path.Path) (map[string]int, error) {
-	tags := map[string]int{}
-	typ := trueReflectValue(in).Type()
+// the fields of the struct `in`. `in` must be a struct.
+//
+// The position of the field in a struct is represented as an index sequence to support type embedding
+// in structs. This index sequence can be used to retrieve the field with the Go "reflect" package FieldByIndex methods:
+//   - https://pkg.go.dev/reflect#Type.FieldByIndex
+//   - https://pkg.go.dev/reflect#Value.FieldByIndex
+//   - https://pkg.go.dev/reflect#Value.FieldByIndexErr
+//
+// The following are not supported and will return an error if detected in a struct (including embedded structs):
+//   - Duplicate "tfsdk" tags
+//   - Exported fields without a "tfsdk" tag
+//   - Exported fields with an invalid "tfsdk" tag (must be a valid Terraform identifier)
+func getStructTags(ctx context.Context, typ reflect.Type, path path.Path) (map[string][]int, error) { //nolint:unparam // False positive, ctx is used below.
+	tags := make(map[string][]int, 0)
+
+	if typ.Kind() == reflect.Pointer {
+		typ = typ.Elem()
+	}
 	if typ.Kind() != reflect.Struct {
-		return nil, fmt.Errorf("%s: can't get struct tags of %s, is not a struct", path, in.Type())
+		return nil, fmt.Errorf("%s: can't get struct tags of %s, is not a struct", path, typ)
 	}
+
 	for i := 0; i < typ.NumField(); i++ {
 		field := typ.Field(i)
-		if field.PkgPath != "" {
-			// skip unexported fields
+		if !field.IsExported() && !field.Anonymous {
+			// Skip unexported fields. Unexported embedded structs (anonymous fields) are allowed because they may
+			// contain exported fields that are promoted; which means they can be read/set.
 			continue
 		}
+
+		// This index sequence is the location of the field within the struct.
+		// For promoted fields from an embedded struct, the length of this sequence will be > 1
+		fieldIndexSequence := []int{i}
 		tag := field.Tag.Get(`tfsdk`)
-		if tag == "-" {
-			// skip explicitly excluded fields
-			continue
-		}
-		if tag == "" {
+
+		switch tag {
+		// "tfsdk" tags can only be omitted on embedded structs
+		case "":
+			if field.Anonymous {
+				embeddedTags, err := getStructTags(ctx, field.Type, path)
+				if err != nil {
+					return nil, fmt.Errorf(`error retrieving embedded struct %q field tags: %w`, field.Name, err)
+				}
+				for k, v := range embeddedTags {
+					if other, ok := tags[k]; ok {
+						otherField := typ.FieldByIndex(other)
+						return nil, fmt.Errorf("embedded struct %q promotes a field with a duplicate tfsdk tag %q, conflicts with %q tfsdk tag", field.Name, k, otherField.Name)
+					}
+
+					tags[k] = append(fieldIndexSequence, v...)
+				}
+				continue
+			}
+
 			return nil, fmt.Errorf(`%s: need a struct tag for "tfsdk" on %s`, path, field.Name)
+
+		// "tfsdk" tags with "-" are being explicitly excluded
+		case "-":
+			continue
+
+		// validate the "tfsdk" tag and ensure there are no duplicates before storing
+		default:
+			path := path.AtName(tag)
+			if field.Anonymous {
+				return nil, fmt.Errorf(`%s: embedded struct field %s cannot have tfsdk tag`, path, field.Name)
+			}
+			if !isValidFieldName(tag) {
+				return nil, fmt.Errorf("%s: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter", path)
+			}
+			if other, ok := tags[tag]; ok {
+				otherField := typ.FieldByIndex(other)
+				return nil, fmt.Errorf("%s: can't use tfsdk tag %q for both %s and %s fields", path, tag, otherField.Name, field.Name)
+			}
+
+			tags[tag] = fieldIndexSequence
 		}
-		path := path.AtName(tag)
-		if !isValidFieldName(tag) {
-			return nil, fmt.Errorf("%s: invalid field name, must only use lowercase letters, underscores, and numbers, and must start with a letter", path)
-		}
-		if other, ok := tags[tag]; ok {
-			return nil, fmt.Errorf("%s: can't use field name for both %s and %s", path, typ.Field(other).Name, field.Name)
-		}
-		tags[tag] = i
 	}
 	return tags, nil
 }

internal/reflect/helpers_test.go

@@ -4,11 +4,241 @@
 package reflect
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/hashicorp/terraform-plugin-framework/path"
 )
 
+type ExampleStruct struct {
+	StrField  string `tfsdk:"str_field"`
+	IntField  int    `tfsdk:"int_field"`
+	BoolField bool   `tfsdk:"bool_field"`
+	IgnoreMe  string `tfsdk:"-"`
+
+	unexported          string //nolint:structcheck,unused
+	unexportedAndTagged string `tfsdk:"unexported_and_tagged"`
+}
+
+type NestedEmbed struct {
+	ListField []string `tfsdk:"list_field"`
+	DoubleNestedEmbed
+}
+
+type DoubleNestedEmbed struct {
+	Map map[string]string `tfsdk:"map_field"`
+	ExampleStruct
+}
+
+type EmbedWithDuplicates struct {
+	StrField1 string `tfsdk:"str_field"`
+	StrField2 string `tfsdk:"str_field"`
+}
+
+type StructWithInvalidTag struct {
+	InvalidField string `tfsdk:"*()-"`
+}
+
+func TestGetStructTags(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		in           any
+		expectedTags map[string][]int
+		expectedErr  error
+	}{
+		"struct": {
+			in: ExampleStruct{},
+			expectedTags: map[string][]int{
+				"str_field":  {0},
+				"int_field":  {1},
+				"bool_field": {2},
+			},
+		},
+		"struct-err-duplicate-fields": {
+			in: struct {
+				StrField string `tfsdk:"str_field"`
+				IntField string `tfsdk:"str_field"`
+			}{},
+			expectedErr: errors.New(`str_field: can't use tfsdk tag "str_field" for both StrField and IntField fields`),
+		},
+		"embedded-struct-err-duplicate-fields": {
+			in: struct {
+				EmbedWithDuplicates
+			}{},
+			expectedErr: errors.New(`error retrieving embedded struct "EmbedWithDuplicates" field tags: str_field: can't use tfsdk tag "str_field" for both StrField1 and StrField2 fields`),
+		},
+		"embedded-struct-err-duplicate-fields-from-promote": {
+			in: struct {
+				StrField      string `tfsdk:"str_field"`
+				ExampleStruct        // Contains a `tfsdk:"str_field"`
+			}{},
+			expectedErr: errors.New(`embedded struct "ExampleStruct" promotes a field with a duplicate tfsdk tag "str_field", conflicts with "StrField" tfsdk tag`),
+		},
+		"struct-err-invalid-field": {
+			in:          StructWithInvalidTag{},
+			expectedErr: errors.New(`*()-: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter`),
+		},
+		"ignore-embedded-struct": {
+			in: struct {
+				ExampleStruct `tfsdk:"-"`
+				Field5        string `tfsdk:"field5"`
+			}{},
+			expectedTags: map[string][]int{
+				"field5": {1},
+			},
+		},
+		"embedded-struct": {
+			in: struct {
+				ExampleStruct
+				Field5 string `tfsdk:"field5"`
+			}{},
+			expectedTags: map[string][]int{
+				"str_field":  {0, 0},
+				"int_field":  {0, 1},
+				"bool_field": {0, 2},
+				"field5":     {1},
+			},
+		},
+		"nested-embedded-struct": {
+			in: struct {
+				NestedEmbed
+				Field5 string `tfsdk:"field5"`
+			}{},
+			expectedTags: map[string][]int{
+				"list_field": {0, 0},
+				"map_field":  {0, 1, 0},
+				"str_field":  {0, 1, 1, 0},
+				"int_field":  {0, 1, 1, 1},
+				"bool_field": {0, 1, 1, 2},
+				"field5":     {1},
+			},
+		},
+		"embedded-struct-unexported": {
+			in: struct {
+				ExampleStruct
+				Field5 string `tfsdk:"field5"`
+
+				unexported          string //nolint:structcheck,unused
+				unexportedAndTagged string `tfsdk:"unexported_and_tagged"`
+			}{},
+			expectedTags: map[string][]int{
+				"str_field":  {0, 0},
+				"int_field":  {0, 1},
+				"bool_field": {0, 2},
+				"field5":     {1},
+			},
+		},
+		"embedded-struct-err-cannot-have-tfsdk-tag": {
+			in: struct {
+				ExampleStruct `tfsdk:"example_field"` // Can't put a tfsdk tag here
+			}{},
+			expectedErr: errors.New(`example_field: embedded struct field ExampleStruct cannot have tfsdk tag`),
+		},
+		"embedded-struct-err-invalid": {
+			in: struct {
+				StructWithInvalidTag // Contains an invalid "tfsdk" tag
+			}{},
+			expectedErr: errors.New(`error retrieving embedded struct "StructWithInvalidTag" field tags: *()-: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter`),
+		},
+		// NOTE: The following tests are for embedded struct pointers, despite them not being explicitly supported by the framework reflect package.
+		// Embedded struct pointers still produce a valid field index, but are later rejected when retrieving them. These tests just ensure that there
+		// are no panics when retrieving the field index for an embedded struct pointer field
+		"ignore-embedded-struct-ptr": {
+			in: struct {
+				*ExampleStruct `tfsdk:"-"`
+				Field5         string `tfsdk:"field5"`
+			}{},
+			expectedTags: map[string][]int{
+				"field5": {1},
+			},
+		},
+		"embedded-struct-ptr": {
+			in: struct {
+				*ExampleStruct
+				Field5 string `tfsdk:"field5"`
+			}{},
+			expectedTags: map[string][]int{
+				"str_field":  {0, 0},
+				"int_field":  {0, 1},
+				"bool_field": {0, 2},
+				"field5":     {1},
+			},
+		},
+		"embedded-struct-ptr-unexported": {
+			in: struct {
+				*ExampleStruct
+				Field5 string `tfsdk:"field5"`
+
+				unexported          string //nolint:structcheck,unused
+				unexportedAndTagged string `tfsdk:"unexported_and_tagged"`
+			}{},
+			expectedTags: map[string][]int{
+				"str_field":  {0, 0},
+				"int_field":  {0, 1},
+				"bool_field": {0, 2},
+				"field5":     {1},
+			},
+		},
+		"embedded-struct-ptr-err-cannot-have-tfsdk-tag": {
+			in: struct {
+				*ExampleStruct `tfsdk:"example_field"` // Can't put a tfsdk tag here
+			}{},
+			expectedErr: errors.New(`example_field: embedded struct field ExampleStruct cannot have tfsdk tag`),
+		},
+		"embedded-struct-ptr-err-duplicate-fields": {
+			in: struct {
+				*EmbedWithDuplicates
+			}{},
+			expectedErr: errors.New(`error retrieving embedded struct "EmbedWithDuplicates" field tags: str_field: can't use tfsdk tag "str_field" for both StrField1 and StrField2 fields`),
+		},
+		"embedded-struct-ptr-err-duplicate-fields-from-promote": {
+			in: struct {
+				StrField       string `tfsdk:"str_field"`
+				*ExampleStruct        // Contains a `tfsdk:"str_field"`
+			}{},
+			expectedErr: errors.New(`embedded struct "ExampleStruct" promotes a field with a duplicate tfsdk tag "str_field", conflicts with "StrField" tfsdk tag`),
+		},
+		"embedded-struct-ptr-err-invalid": {
+			in: struct {
+				*StructWithInvalidTag // Contains an invalid "tfsdk" tag
+			}{},
+			expectedErr: errors.New(`error retrieving embedded struct "StructWithInvalidTag" field tags: *()-: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter`),
+		},
+	}
+
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			tags, err := getStructTags(context.Background(), reflect.TypeOf(testCase.in), path.Empty())
+			if err != nil {
+				if testCase.expectedErr == nil {
+					t.Fatalf("expected no error, got: %s", err)
+				}
+
+				if !strings.Contains(err.Error(), testCase.expectedErr.Error()) {
+					t.Fatalf("expected error %q, got: %s", testCase.expectedErr, err)
+				}
+			}
+
+			if err == nil && testCase.expectedErr != nil {
+				t.Fatalf("got no error, expected: %s", testCase.expectedErr)
+			}
+
+			if diff := cmp.Diff(tags, testCase.expectedTags); diff != "" {
+				t.Errorf("unexpected difference: %s", diff)
+			}
+		})
+	}
+}
+
 func TestTrueReflectValue(t *testing.T) {
 	t.Parallel()