internal/sdk: resource decoding/encoding now parses the struct tags consistently

internal/sdk/resource_decode.go

@@ -6,7 +6,6 @@ package sdk
 import (
 	"fmt"
 	"reflect"
-	"strings"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
@@ -57,9 +56,13 @@ func decodeReflectedType(input interface{}, stateRetriever stateRetriever, debug
 		field := objType.Field(i)
 		debugLogger.Infof("Field", field)
 
-		if val, exists := field.Tag.Lookup("tfschema"); exists {
-			val = strings.TrimSuffix(val, ",removedInNextMajorVersion")
-			tfschemaValue, valExists := stateRetriever.GetOkExists(val)
+		structTags, err := parseStructTags(field.Tag)
+		if err != nil {
+			return fmt.Errorf("parsing struct tags for %q: %+v", field.Name, err)
+		}
+
+		if structTags != nil {
+			tfschemaValue, valExists := stateRetriever.GetOkExists(structTags.hclPath)
 			if !valExists {
 				continue
 			}
@@ -193,9 +196,13 @@ func setListValue(input interface{}, index int, fieldName string, v []interface{
 					nestedField := elem.Type().Elem().Field(j)
 					debugLogger.Infof("nestedField ", nestedField)
 
-					if val, exists := nestedField.Tag.Lookup("tfschema"); exists {
-						val = strings.TrimSuffix(val, ",removedInNextMajorVersion")
-						nestedTFSchemaValue := test[val]
+					structTags, err := parseStructTags(nestedField.Tag)
+					if err != nil {
+						return fmt.Errorf("parsing struct tags for nested field %q: %+v", nestedField.Name, err)
+					}
+
+					if structTags != nil {
+						nestedTFSchemaValue := test[structTags.hclPath]
 						if err := setValue(elem.Interface(), nestedTFSchemaValue, j, fieldName, debugLogger); err != nil {
 							return err
 						}

internal/sdk/resource_encode.go

@@ -6,7 +6,6 @@ package sdk
 import (
 	"fmt"
 	"reflect"
-	"strings"
 
 	"github.com/hashicorp/terraform-provider-azurerm/internal/features"
 )
@@ -54,71 +53,80 @@ func recurse(objType reflect.Type, objVal reflect.Value, fieldName string, debug
 	for i := 0; i < objType.NumField(); i++ {
 		field := objType.Field(i)
 		fieldVal := objVal.Field(i)
-		if tfschemaTag, exists := field.Tag.Lookup("tfschema"); exists && !(strings.Contains(tfschemaTag, "removedInNextMajorVersion") && features.FourPointOh()) {
-			tfschemaTag = strings.TrimSuffix(tfschemaTag, ",removedInNextMajorVersion")
+		structTags, err := parseStructTags(field.Tag)
+		if err != nil {
+			return nil, fmt.Errorf("parsing struct tags for %q: %+v", field.Name, err)
+		}
+
+		if structTags != nil {
+			if structTags.removedInNextMajorVersion && features.FourPointOh() {
+				debugLogger.Infof("The HCL Path %q is marked as removed - skipping", structTags.hclPath)
+				continue
+			}
+
 			switch field.Type.Kind() {
 			case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
 				iv := fieldVal.Int()
-				debugLogger.Infof("Setting %q to %d", tfschemaTag, iv)
-				output[tfschemaTag] = iv
+				debugLogger.Infof("Setting %q to %d", structTags.hclPath, iv)
+				output[structTags.hclPath] = iv
 
 			case reflect.Float32, reflect.Float64:
 				fv := fieldVal.Float()
-				debugLogger.Infof("Setting %q to %f", tfschemaTag, fv)
-				output[tfschemaTag] = fv
+				debugLogger.Infof("Setting %q to %f", structTags.hclPath, fv)
+				output[structTags.hclPath] = fv
 
 			case reflect.String:
 				sv := fieldVal.String()
-				debugLogger.Infof("Setting %q to %q", tfschemaTag, sv)
-				output[tfschemaTag] = sv
+				debugLogger.Infof("Setting %q to %q", structTags.hclPath, sv)
+				output[structTags.hclPath] = sv
 
 			case reflect.Bool:
 				bv := fieldVal.Bool()
-				debugLogger.Infof("Setting %q to %t", tfschemaTag, bv)
-				output[tfschemaTag] = bv
+				debugLogger.Infof("Setting %q to %t", structTags.hclPath, bv)
+				output[structTags.hclPath] = bv
 
 			case reflect.Map:
 				iter := fieldVal.MapRange()
 				attr := make(map[string]interface{})
 				for iter.Next() {
 					attr[iter.Key().String()] = iter.Value().Interface()
 				}
-				output[tfschemaTag] = attr
+				output[structTags.hclPath] = attr
 
 			case reflect.Slice:
 				sv := fieldVal.Slice(0, fieldVal.Len())
 				attr := make([]interface{}, sv.Len())
 				switch sv.Type() {
 				case reflect.TypeOf([]string{}):
-					debugLogger.Infof("Setting %q to []string", tfschemaTag)
+					debugLogger.Infof("Setting %q to []string", structTags.hclPath)
 					if sv.Len() > 0 {
-						output[tfschemaTag] = sv.Interface()
+						output[structTags.hclPath] = sv.Interface()
 					} else {
-						output[tfschemaTag] = make([]string, 0)
+						output[structTags.hclPath] = make([]string, 0)
 					}
 
 				case reflect.TypeOf([]int{}):
-					debugLogger.Infof("Setting %q to []int", tfschemaTag)
+					debugLogger.Infof("Setting %q to []int", structTags.hclPath)
 					if sv.Len() > 0 {
-						output[tfschemaTag] = sv.Interface()
+						output[structTags.hclPath] = sv.Interface()
 					} else {
-						output[tfschemaTag] = make([]int, 0)
+						output[structTags.hclPath] = make([]int, 0)
 					}
 
 				case reflect.TypeOf([]float64{}):
-					debugLogger.Infof("Setting %q to []float64", tfschemaTag)
+					debugLogger.Infof("Setting %q to []float64", structTags.hclPath)
 					if sv.Len() > 0 {
-						output[tfschemaTag] = sv.Interface()
+						output[structTags.hclPath] = sv.Interface()
 					} else {
-						output[tfschemaTag] = make([]float64, 0)
+						output[structTags.hclPath] = make([]float64, 0)
 					}
 
 				case reflect.TypeOf([]bool{}):
-					debugLogger.Infof("Setting %q to []bool", tfschemaTag)
+					debugLogger.Infof("Setting %q to []bool", structTags.hclPath)
 					if sv.Len() > 0 {
-						output[tfschemaTag] = sv.Interface()
+						output[structTags.hclPath] = sv.Interface()
 					} else {
-						output[tfschemaTag] = make([]bool, 0)
+						output[structTags.hclPath] = make([]bool, 0)
 					}
 
 				default:
@@ -135,11 +143,11 @@ func recurse(objType reflect.Type, objVal reflect.Value, fieldName string, debug
 						}
 						attr[i] = serialized
 					}
-					debugLogger.Infof("[SLICE] Setting %q to %+v", tfschemaTag, attr)
-					output[tfschemaTag] = attr
+					debugLogger.Infof("[SLICE] Setting %q to %+v", structTags.hclPath, attr)
+					output[structTags.hclPath] = attr
 				}
 			default:
-				return output, fmt.Errorf("unknown type %+v for key %q", field.Type.Kind(), tfschemaTag)
+				return output, fmt.Errorf("unknown type %+v for key %q", field.Type.Kind(), structTags.hclPath)
 			}
 		}
 	}

internal/sdk/resource_helpers.go

@@ -0,0 +1,55 @@
+package sdk
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+)
+
+type decodedStructTags struct {
+	// hclPath defines the path to this field used for this in the Schema for this Resource
+	hclPath string
+
+	// removedInNextMajorVersion specifies whether this field is deprecated and should not
+	// be set into the state in the next major version of the Provider
+	removedInNextMajorVersion bool
+}
+
+// parseStructTags parses the struct tags defined in input into a decodedStructTags object
+// which allows for the consistent parsing of struct tags across the Typed SDK.
+func parseStructTags(input reflect.StructTag) (*decodedStructTags, error) {
+	tag, ok := input.Lookup("tfschema")
+	if !ok {
+		// doesn't exist - ignore it?
+		return nil, nil
+	}
+	if tag == "" {
+		return nil, fmt.Errorf("the `tfschema` struct tag was defined but empty")
+	}
+
+	components := strings.Split(tag, ",")
+	output := &decodedStructTags{
+		// NOTE: `hclPath` has to be the first item in the struct tag
+		hclPath:                   strings.TrimSpace(components[0]),
+		removedInNextMajorVersion: false,
+	}
+	if output.hclPath == "" {
+		return nil, fmt.Errorf("hclPath was empty")
+	}
+
+	if len(components) > 1 {
+		// remove the hcl field name since it's been parsed
+		components = components[1:]
+		for _, item := range components {
+			item = strings.TrimSpace(item) // allowing for both `foo,bar` and `foo, bar` in struct tags
+			if strings.EqualFold(item, "removedInNextMajorVersion") {
+				output.removedInNextMajorVersion = true
+				continue
+			}
+
+			return nil, fmt.Errorf("internal-error: the struct-tag %q is not implemented - struct tags are %q", item, tag)
+		}
+	}
+
+	return output, nil
+}

internal/sdk/resource_helpers_test.go

@@ -0,0 +1,111 @@
+package sdk
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/hashicorp/go-azure-helpers/lang/pointer"
+)
+
+func TestParseStructTags_Empty(t *testing.T) {
+	actual, err := parseStructTags("")
+	if err != nil {
+		t.Fatalf("unexpected error %q", err.Error())
+	}
+
+	if actual != nil {
+		t.Fatalf("expected actual to be nil but got %+v", *actual)
+	}
+}
+
+func TestParseStructTags_WithValue(t *testing.T) {
+	testData := []struct {
+		input    reflect.StructTag
+		expected *decodedStructTags
+		error    *string
+	}{
+		{
+			// empty hclPath
+			input:    `tfschema:""`,
+			expected: nil,
+			error:    pointer.To("the `tfschema` struct tag was defined but empty"),
+		},
+		{
+			// valid, no removedInNextMajorVersion
+			input: `tfschema:"hello"`,
+			expected: &decodedStructTags{
+				hclPath:                   "hello",
+				removedInNextMajorVersion: false,
+			},
+		},
+		{
+			// valid, with removedInNextMajorVersion
+			input: `tfschema:"hello,removedInNextMajorVersion"`,
+			expected: &decodedStructTags{
+				hclPath:                   "hello",
+				removedInNextMajorVersion: true,
+			},
+		},
+		{
+			// valid, with removedInNextMajorVersion and a space before the comma
+			input: `tfschema:"hello, removedInNextMajorVersion"`,
+			expected: &decodedStructTags{
+				hclPath:                   "hello",
+				removedInNextMajorVersion: true,
+			},
+		},
+		{
+			// valid, with removedInNextMajorVersion and a space after the comma
+			//
+			// This would be caught in PR review, but would be a confusing error/experience
+			// during development so it's worth being lenient here since it's non-impactful
+			input: `tfschema:"hello ,removedInNextMajorVersion"`,
+			expected: &decodedStructTags{
+				hclPath:                   "hello",
+				removedInNextMajorVersion: true,
+			},
+		},
+		{
+			// valid, with removedInNextMajorVersion and a space either side
+			//
+			// This would be caught in PR review, but would be a confusing error/experience
+			// during development so it's worth being lenient here since it's non-impactful
+			input: `tfschema:"hello , removedInNextMajorVersion"`,
+			expected: &decodedStructTags{
+				hclPath:                   "hello",
+				removedInNextMajorVersion: true,
+			},
+		},
+		{
+			// invalid, unknown struct tags
+			input:    `tfschema:"hello,world"`,
+			expected: nil,
+			error:    pointer.To(`internal-error: the struct-tag "world" is not implemented - struct tags are "hello,world"`),
+		},
+	}
+	for i, data := range testData {
+		t.Logf("Index %d - Input %q", i, data.input)
+		actual, err := parseStructTags(data.input)
+		if err != nil {
+			if data.error != nil {
+				if err.Error() == *data.error {
+					continue
+				}
+
+				t.Fatalf("expected the error %q but got %q", *data.error, err.Error())
+			}
+
+			t.Fatalf("unexpected error %q", err.Error())
+		}
+		if data.error != nil {
+			t.Fatalf("expected the error %q but didn't get one", *data.error)
+		}
+
+		if actual == nil {
+			t.Fatalf("expected actual to have a value but got nil")
+		}
+		if !reflect.DeepEqual(*data.expected, *actual) {
+			t.Fatalf("expected [%+v] and actual [%+v] didn't match", *data.expected, *actual)
+		}
+	}
+}

internal/sdk/wrapper_validate.go

@@ -59,9 +59,13 @@ func validateModelObjectRecursively(prefix string, objType reflect.Type, objVal
 			}
 		}
 
-		if _, exists := field.Tag.Lookup("tfschema"); !exists {
-			fieldName := strings.TrimPrefix(fmt.Sprintf("%s.%s", prefix, field.Name), ".")
-			return fmt.Errorf("field %q is missing an `tfschema` label", fieldName)
+		fieldName := strings.TrimPrefix(fmt.Sprintf("%s.%s", prefix, field.Name), ".")
+		structTags, err := parseStructTags(field.Tag)
+		if err != nil {
+			return fmt.Errorf("parsing struct tags for %q", fieldName)
+		}
+		if structTags == nil {
+			return fmt.Errorf("field %q is missing a struct tag for `tfschema`", fieldName)
 		}
 	}