Skip to content

Commit

Permalink
refactor to use Tag.Lookup and add tests for empty tags
Browse files Browse the repository at this point in the history
  • Loading branch information
@austinvalle
austinvalle committed Aug 1, 2024
1 parent 2be48eb commit 94de60d
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 34 deletions.
72 changes: 38 additions & 34 deletions internal/reflect/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,50 +79,54 @@ func getStructTags(ctx context.Context, typ reflect.Type, path path.Path) (map[s
// 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`)

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)
tag, tagExists := field.Tag.Lookup(`tfsdk`)

// "tfsdk" tags with "-" are being explicitly excluded
case "-":
if tag == "-" {
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)
// Handle embedded structs
if field.Anonymous {
if tagExists {
return nil, fmt.Errorf(`%s: embedded struct field %s cannot have tfsdk tag`, path.AtName(tag), 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)

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)
}
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)
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
}

// All non-embedded fields must have a tfsdk tag
if !tagExists {
return nil, fmt.Errorf(`%s: need a struct tag for "tfsdk" on %s`, path, field.Name)
}

tags[tag] = fieldIndexSequence
// Ensure the tfsdk tag has a valid name
path := path.AtName(tag)
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)
}

// Ensure there are no duplicate tfsdk tags
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
}

return tags, nil
}

Expand Down
24 changes: 24 additions & 0 deletions internal/reflect/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ func TestGetStructTags(t *testing.T) {
in: StructWithInvalidTag{},
expectedErr: errors.New(`*()-: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter`),
},
"struct-err-missing-tfsdk-tag": {
in: struct {
ExampleField string
}{},
expectedErr: errors.New(`: need a struct tag for "tfsdk" on ExampleField`),
},
"struct-err-empty-tfsdk-tag": {
in: struct {
ExampleField string `tfsdk:""`
}{},
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:"-"`
Expand Down Expand Up @@ -134,6 +146,12 @@ func TestGetStructTags(t *testing.T) {
"field5": {1},
},
},
"embedded-struct-err-cannot-have-empty-tfsdk-tag": {
in: struct {
ExampleStruct `tfsdk:""` // Can't put a tfsdk tag here
}{},
expectedErr: errors.New(`: embedded struct field ExampleStruct cannot have tfsdk tag`),
},
"embedded-struct-err-cannot-have-tfsdk-tag": {
in: struct {
ExampleStruct `tfsdk:"example_field"` // Can't put a tfsdk tag here
Expand Down Expand Up @@ -185,6 +203,12 @@ func TestGetStructTags(t *testing.T) {
"field5": {1},
},
},
"embedded-struct-ptr-err-cannot-have-empty-tfsdk-tag": {
in: struct {
*ExampleStruct `tfsdk:""` // Can't put a tfsdk tag here
}{},
expectedErr: errors.New(`: embedded struct field ExampleStruct cannot have tfsdk tag`),
},
"embedded-struct-ptr-err-cannot-have-tfsdk-tag": {
in: struct {
*ExampleStruct `tfsdk:"example_field"` // Can't put a tfsdk tag here
Expand Down
82 changes: 82 additions & 0 deletions internal/reflect/struct_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,26 @@ func TestNewStruct_errors(t *testing.T) {
"error retrieving field names from struct tags: %w",
errors.New("invalidTag: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter")),
},
"struct-has-empty-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
"a": types.StringType,
},
},
objVal: tftypes.NewValue(tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"a": tftypes.String,
},
}, map[string]tftypes.Value{
"a": tftypes.NewValue(tftypes.String, "hello"),
}),
targetVal: reflect.ValueOf(struct {
A string `tfsdk:""`
}{}),
expectedError: fmt.Errorf(
"error retrieving field names from struct tags: %w",
errors.New(": invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter")),
},
"embedded-struct-has-invalid-tags": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
Expand Down Expand Up @@ -332,6 +352,26 @@ func TestNewStruct_errors(t *testing.T) {
"Error: reflect: indirection through nil pointer to embedded struct field embedSingleField",
),
},
"embedded-struct-has-empty-tfsdk-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
"attr_1": types.StringType,
},
},
objVal: tftypes.NewValue(tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"attr_1": tftypes.String,
},
}, map[string]tftypes.Value{
"attr_1": tftypes.NewValue(tftypes.String, "hello"),
}),
targetVal: reflect.ValueOf(struct {
embedSingleField `tfsdk:""`
}{}),
expectedError: errors.New(
"error retrieving field names from struct tags: : embedded struct field embedSingleField cannot have tfsdk tag",
),
},
"embedded-struct-has-tfsdk-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
Expand Down Expand Up @@ -1803,6 +1843,27 @@ func TestFromStruct_errors(t *testing.T) {
),
},
},
"struct-has-empty-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
"test": types.StringType,
},
},
val: reflect.ValueOf(
struct {
Test types.String `tfsdk:""`
}{},
),
expectedDiags: diag.Diagnostics{
diag.NewAttributeErrorDiagnostic(
path.Root("test"),
"Value Conversion Error",
"An unexpected error was encountered trying to convert from struct value. "+
"This is always an error in the provider. Please report the following to the provider developer:\n\n"+
"error retrieving field names from struct tags: test.: invalid tfsdk tag, must only use lowercase letters, underscores, and numbers, and must start with a letter",
),
},
},
"embedded-struct-has-invalid-tags": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
Expand Down Expand Up @@ -2016,6 +2077,27 @@ func TestFromStruct_errors(t *testing.T) {
),
},
},
"embedded-struct-has-empty-tfsdk-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
"attr_1": types.StringType,
},
},
val: reflect.ValueOf(
struct {
embedSingleField `tfsdk:""`
}{},
),
expectedDiags: diag.Diagnostics{
diag.NewAttributeErrorDiagnostic(
path.Root("test"),
"Value Conversion Error",
"An unexpected error was encountered trying to convert from struct value. "+
"This is always an error in the provider. Please report the following to the provider developer:\n\n"+
"error retrieving field names from struct tags: test.: embedded struct field embedSingleField cannot have tfsdk tag",
),
},
},
"embedded-struct-has-tfsdk-tag": {
typ: types.ObjectType{
AttrTypes: map[string]attr.Type{
Expand Down

0 comments on commit 94de60d

Please sign in to comment.