Explore changing SetTagsDiff

internal/verify/diff.go

@@ -45,22 +45,11 @@ func SetTagsDiff(ctx context.Context, diff *schema.ResourceDiff, meta interface{
 	}
 
 	if diff.HasChange("tags") {
-		_, n := diff.GetChange("tags")
-		newTags := tftags.New(ctx, n.(map[string]interface{}))
-
-		if newTags.HasZeroValue() {
-			if err := diff.SetNewComputed("tags_all"); err != nil {
-				return fmt.Errorf("setting tags_all to computed: %w", err)
-			}
-		}
-
-		if len(allTags) > 0 && (!newTags.HasZeroValue() || !allTags.HasZeroValue()) {
+		if len(allTags) > 0 {
 			if err := diff.SetNew("tags_all", allTags.Map()); err != nil {
 				return fmt.Errorf("setting new tags_all diff: %w", err)
 			}
-		}
-
-		if len(allTags) == 0 {
+		} else if len(allTags) == 0 {
 			if err := diff.SetNewComputed("tags_all"); err != nil {
 				return fmt.Errorf("setting tags_all to computed: %w", err)
 			}

internal/verify/diff_test.go

@@ -4,11 +4,17 @@
 package verify
 
 import (
+	"context"
 	"reflect"
 	"testing"
 	"time"
 
+	"github.com/hashicorp/go-cty/cty"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/hashicorp/terraform-provider-aws/internal/conns"
 	"github.com/hashicorp/terraform-provider-aws/internal/flex"
+	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
 )
 
 func TestSuppressEquivalentRoundedTime(t *testing.T) {
@@ -162,3 +168,179 @@ func TestDiffStringMaps(t *testing.T) {
 		}
 	}
 }
+
+func TestSetTagsDiff(t *testing.T) {
+
+	type testCase struct {
+		name              string
+		state             cty.Value
+		config            cty.Value
+		defaultTagsConfig *tftags.DefaultConfig
+		ignoreTagsConfig  *tftags.IgnoreConfig
+		expectedTagsAll   cty.Value
+		expectedNoTagsAll bool
+	}
+
+	testCases := []testCase{
+		{
+			name: "creating with no tags",
+			state: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapValEmpty(cty.String),
+			}),
+			config: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapValEmpty(cty.String),
+			}),
+			// This behavior is strange, why is this the answer unknown instead of not
+			// setting the tags_all at all?
+			expectedTagsAll: cty.UnknownVal(cty.Map(cty.String)),
+		},
+		{
+			name:  "basic tags get copied to tags_all",
+			state: cty.ObjectVal(map[string]cty.Value{}),
+			config: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapVal(map[string]cty.Value{
+					"tag1": cty.StringVal("tag1v"),
+				}),
+			}),
+			expectedTagsAll: cty.MapVal(map[string]cty.Value{
+				"tag1": cty.StringVal("tag1v"),
+			}),
+		},
+		{
+			name: "basic tags not changing get copied to tags_all",
+			state: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapVal(map[string]cty.Value{
+					"tag1": cty.StringVal("tag1v"),
+				}),
+			}),
+			config: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapVal(map[string]cty.Value{
+					"tag1": cty.StringVal("tag1v"),
+				}),
+			}),
+			expectedTagsAll: cty.MapVal(map[string]cty.Value{
+				"tag1": cty.StringVal("tag1v"),
+			}),
+		},
+		{
+			name:  "unknowns in tags plan cause unknown tags_all",
+			state: cty.ObjectVal(map[string]cty.Value{}),
+			config: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.UnknownVal(cty.Map(cty.String)),
+			}),
+			expectedTagsAll: cty.UnknownVal(cty.Map(cty.String)),
+		},
+		{
+			name:  "setting a simple empty-value tag works as expected",
+			state: cty.ObjectVal(map[string]cty.Value{}),
+			config: cty.ObjectVal(map[string]cty.Value{
+				"tags": cty.MapVal(map[string]cty.Value{
+					"tag1": cty.StringVal(""),
+				}),
+			}),
+			expectedTagsAll: cty.MapVal(map[string]cty.Value{
+				// This is really not right. It looks like SetTagsDiff can now copy
+				// tags to tags_all, but because tag1 is empty and tags_all is a
+				// computed attribute, it activates finalizeDiff clause that decides that this empty value
+				// is really unknown after all, causing this problem!
+				//
+				// https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L534
+				"tag1": cty.UnknownVal(cty.String),
+			}),
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			d, err := checkSetTagsDiff(tc.state, tc.config, tc.defaultTagsConfig, tc.ignoreTagsConfig)
+			if err != nil {
+				t.Error(err)
+				t.FailNow()
+			}
+
+			_, actualGotTagsAll := d.GetAttribute("tags_all.%")
+			if actualGotTagsAll != !tc.expectedNoTagsAll {
+				t.Logf("A = %v", d.Attributes)
+				for k, v := range d.Attributes {
+					t.Logf("%v => %v Computed=%v", k, v, v.NewComputed)
+
+				}
+				t.Errorf("tags_all handled incorrectly\n  expected to be set: %v\n  was set: %v\n",
+					!tc.expectedNoTagsAll,
+					actualGotTagsAll,
+				)
+			}
+
+			if tc.expectedNoTagsAll {
+				return
+			}
+
+			applied, err := d.ApplyToValue(tc.state, resourceWithTags().CoreConfigSchema())
+			if err != nil {
+				t.Error(err)
+				t.FailNow()
+			}
+
+			actual := applied.GetAttr("tags_all")
+			expected := tc.expectedTagsAll
+
+			if !reflect.DeepEqual(actual, expected) {
+				t.Errorf("tags_all not set as expected\n  actual: %v\n  expected: %v\n",
+					actual.GoString(),
+					expected.GoString(),
+				)
+				t.FailNow()
+			}
+		})
+	}
+}
+
+func resourceWithTags() *schema.Resource {
+	return &schema.Resource{
+		Schema: map[string]*schema.Schema{
+			"tags":     tagsSchema(),
+			"tags_all": tagsSchemaTrulyComputed(),
+		},
+	}
+}
+
+func checkSetTagsDiff(
+	state cty.Value,
+	config cty.Value,
+	defaultTagsConfig *tftags.DefaultConfig,
+	ignoreTagsConfig *tftags.IgnoreConfig,
+) (*terraform.InstanceDiff, error) {
+	ctx := context.Background()
+	resource := resourceWithTags()
+	instanceState := terraform.NewInstanceStateShimmedFromValue(state, 0)
+	// Usually TF CLI sets RawPlan to a merge of state and config, but it does not matter for this test.
+	instanceState.RawPlan = config
+	resourceConfig := terraform.NewResourceConfigShimmed(config, resource.CoreConfigSchema())
+	meta := &conns.AWSClient{}
+	meta.DefaultTagsConfig = defaultTagsConfig
+	meta.IgnoreTagsConfig = ignoreTagsConfig
+	m := schema.InternalMap(resource.Schema)
+	diff, err := m.Diff(ctx, instanceState, resourceConfig, SetTagsDiff, meta, false /* handleRequiresNew */)
+	if err != nil {
+		return nil, err
+	}
+	return diff, nil
+}
+
+func tagsSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeMap,
+		Optional: true,
+		Elem:     &schema.Schema{Type: schema.TypeString},
+	}
+}
+
+func tagsSchemaTrulyComputed() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeMap,
+		Computed: true,
+		Elem:     &schema.Schema{Type: schema.TypeString},
+	}
+}