From fdaa221f2a81f537ed8506867a34dace6fafe54c Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 20 Apr 2021 19:43:20 -0400 Subject: [PATCH] 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: https://github.com/hashicorp/terraform-provider-aws/pull/18726#discussion_r616311019 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 | 27 +-- .../keyvaluetags/key_value_tags_test.go | 154 ++++++++++++++++++ 2 files changed, 169 insertions(+), 12 deletions(-) diff --git a/aws/internal/keyvaluetags/key_value_tags.go b/aws/internal/keyvaluetags/key_value_tags.go index 1f48f3c1cd6..f857684c326 100644 --- a/aws/internal/keyvaluetags/key_value_tags.go +++ b/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 diff --git a/aws/internal/keyvaluetags/key_value_tags_test.go b/aws/internal/keyvaluetags/key_value_tags_test.go index 39717e69757..e8aa303d3f6 100644 --- a/aws/internal/keyvaluetags/key_value_tags_test.go +++ b/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