internal/keyvaluetags: Allow KeyValueTags type passthrough in New(), …

…add unit testing, and fix potential panic (#19011)

* internal/keyvaluetags: Allow KeyValueTags type passthrough in New(), add unit testing, and fix potential panic

Reference: #18726 (comment)

The current behavior is to return an empty `KeyValueTags` if the type is unrecognized in `New()` in preference over panicking or requiring all consumers to implement error handling. This augments `New()` to accept the named `KeyValueTags` in addition to its underlying `map[string]*TagData` type, preventing confusing behavior on an otherwise easy to miss resource/service implementation detail.

Added unit testing to cover various `New()` type scenarios. Fixed panic discovered while implementing the unit testing.

* internal/keyvaluetags: Ensure New() of KeyValueTags named and underlying types are copies

aws/internal/keyvaluetags/key_value_tags.go

@@ -491,20 +491,18 @@ func (tags KeyValueTags) UrlEncode() string {
 	return values.Encode()
 }
 
-// New creates KeyValueTags from common Terraform Provider SDK types.
-// Supports map[string]string, map[string]*string, map[string]interface{}, and []interface{}.
+// New creates KeyValueTags from common types or returns an empty KeyValueTags.
+//
+// Supports various Terraform Plugin SDK types including map[string]string,
+// map[string]*string, map[string]interface{}, and []interface{}.
 // When passed []interface{}, all elements are treated as keys and assigned nil values.
+// When passed KeyValueTags or its underlying type implementation, returns itself.
 func New(i interface{}) KeyValueTags {
 	switch value := i.(type) {
+	case KeyValueTags:
+		return make(KeyValueTags).Merge(value)
 	case map[string]*TagData:
-		kvtm := make(KeyValueTags, len(value))
-
-		for k, v := range value {
-			tagData := v
-			kvtm[k] = tagData
-		}
-
-		return kvtm
+		return make(KeyValueTags).Merge(KeyValueTags(value))
 	case map[string]string:
 		kvtm := make(KeyValueTags, len(value))
 
@@ -533,8 +531,13 @@ func New(i interface{}) KeyValueTags {
 		kvtm := make(KeyValueTags, len(value))
 
 		for k, v := range value {
-			str := v.(string)
-			kvtm[k] = &TagData{Value: &str}
+			kvtm[k] = &TagData{}
+
+			str, ok := v.(string)
+
+			if ok {
+				kvtm[k].Value = &str
+			}
 		}
 
 		return kvtm

aws/internal/keyvaluetags/key_value_tags_test.go

@@ -2011,6 +2011,160 @@ func TestKeyValueTagsUrlEncode(t *testing.T) {
 	}
 }
 
+func TestNew(t *testing.T) {
+	testCases := []struct {
+		name   string
+		source interface{}
+		want   map[string]string
+	}{
+		{
+			name:   "empty_KeyValueTags",
+			source: KeyValueTags{},
+			want:   map[string]string{},
+		},
+		{
+			name:   "empty_map_string_TagDataPointer",
+			source: map[string]*TagData{},
+			want:   map[string]string{},
+		},
+		{
+			name:   "empty_map_string_interface",
+			source: map[string]interface{}{},
+			want:   map[string]string{},
+		},
+		{
+			name:   "empty_map_string_string",
+			source: map[string]string{},
+			want:   map[string]string{},
+		},
+		{
+			name:   "empty_map_string_stringPointer",
+			source: map[string]*string{},
+			want:   map[string]string{},
+		},
+		{
+			name:   "empty_slice_interface",
+			source: []interface{}{},
+			want:   map[string]string{},
+		},
+		{
+			name: "non_empty_KeyValueTags",
+			source: KeyValueTags{
+				"key1": &TagData{
+					Value: nil,
+				},
+				"key2": &TagData{
+					Value: testStringPtr(""),
+				},
+				"key3": &TagData{
+					Value: testStringPtr("value3"),
+				},
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "",
+				"key3": "value3",
+			},
+		},
+		{
+			name: "non_empty_map_string_TagDataPointer",
+			source: map[string]*TagData{
+				"key1": {
+					Value: nil,
+				},
+				"key2": {
+					Value: testStringPtr(""),
+				},
+				"key3": {
+					Value: testStringPtr("value3"),
+				},
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "",
+				"key3": "value3",
+			},
+		},
+		{
+			name: "non_empty_map_string_interface",
+			source: map[string]interface{}{
+				"key1": nil,
+				"key2": "",
+				"key3": "value3",
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "",
+				"key3": "value3",
+			},
+		},
+		{
+			name: "non_empty_map_string_string",
+			source: map[string]string{
+				"key1": "",
+				"key2": "value2",
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "value2",
+			},
+		},
+		{
+			name: "non_empty_map_string_stringPointer",
+			source: map[string]*string{
+				"key1": nil,
+				"key2": testStringPtr(""),
+				"key3": testStringPtr("value3"),
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "",
+				"key3": "value3",
+			},
+		},
+		{
+			name: "non_empty_slice_interface",
+			source: []interface{}{
+				"key1",
+				"key2",
+			},
+			want: map[string]string{
+				"key1": "",
+				"key2": "",
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			got := New(testCase.source)
+
+			testKeyValueTagsVerifyMap(t, got.Map(), testCase.want)
+
+			// Verify that any source KeyTagValues types are copied
+			// Unfortunately must be done for each separate type
+			switch src := testCase.source.(type) {
+			case KeyValueTags:
+				src.Merge(New(map[string]string{"mergekey": "mergevalue"}))
+
+				_, ok := got.Map()["mergekey"]
+
+				if ok {
+					t.Fatal("expected source to be copied, got source modification")
+				}
+			case map[string]*TagData:
+				src["mergekey"] = &TagData{Value: testStringPtr("mergevalue")}
+
+				_, ok := got.Map()["mergekey"]
+
+				if ok {
+					t.Fatal("expected source to be copied, got source modification")
+				}
+			}
+		})
+	}
+}
+
 func TestTagDataEqual(t *testing.T) {
 	testCases := []struct {
 		name     string