Structured Plan Renderer: Fix minor bugs causing diffs in the equivalence tests. (#32519)

* remove attributes that do not match the relevant attributes filter

* fix formatting

* fix renderer function, don't drop irrelevant attributes just mark them as no-ops

* fix imports

* fix bugs in the renderer exposed by the equivalence tests

* imports

* gofmt

internal/command/jsonformat/computed/renderers/block.go

@@ -57,7 +57,7 @@ func (renderer blockRenderer) RenderHuman(diff computed.Diff, indent int, opts c
 	escapedAttributeKeys := make(map[string]string)
 	for key := range renderer.attributes {
 		attributeKeys = append(attributeKeys, key)
-		escapedKey := ensureValidAttributeName(key)
+		escapedKey := EnsureValidAttributeName(key)
 		escapedAttributeKeys[key] = escapedKey
 		if maximumAttributeKeyLen < len(escapedKey) {
 			maximumAttributeKeyLen = len(escapedKey)
@@ -141,7 +141,7 @@ func (renderer blockRenderer) RenderHuman(diff computed.Diff, indent int, opts c
 			for _, warning := range diff.WarningsHuman(indent+1, blockOpts) {
 				buf.WriteString(fmt.Sprintf("%s%s\n", formatIndent(indent+1), warning))
 			}
-			buf.WriteString(fmt.Sprintf("%s%s %s%s %s\n", formatIndent(indent+1), colorizeDiffAction(diff.Action, blockOpts), ensureValidAttributeName(key), mapKey, diff.RenderHuman(indent+1, blockOpts)))
+			buf.WriteString(fmt.Sprintf("%s%s %s%s %s\n", formatIndent(indent+1), colorizeDiffAction(diff.Action, blockOpts), EnsureValidAttributeName(key), mapKey, diff.RenderHuman(indent+1, blockOpts)))
 
 		}
 

internal/command/jsonformat/computed/renderers/object.go

@@ -50,7 +50,7 @@ func (renderer objectRenderer) RenderHuman(diff computed.Diff, indent int, opts
 	escapedKeys := make(map[string]string)
 	for key := range renderer.attributes {
 		keys = append(keys, key)
-		escapedKey := ensureValidAttributeName(key)
+		escapedKey := EnsureValidAttributeName(key)
 		escapedKeys[key] = escapedKey
 		if maximumKeyLen < len(escapedKey) {
 			maximumKeyLen = len(escapedKey)

internal/command/jsonformat/computed/renderers/primitive.go

@@ -177,12 +177,14 @@ func (renderer primitiveRenderer) renderStringDiff(diff computed.Diff, indent in
 }
 
 func (renderer primitiveRenderer) renderStringDiffAsJson(diff computed.Diff, indent int, opts computed.RenderHumanOpts, before evaluatedString, after evaluatedString) string {
-	jsonDiff := RendererJsonOpts().Transform(before.Json, after.Json, attribute_path.AlwaysMatcher())
+	jsonDiff := RendererJsonOpts().Transform(before.Json, after.Json, diff.Action != plans.Create, diff.Action != plans.Delete, attribute_path.AlwaysMatcher())
 
 	action := diff.Action
 
-	var whitespace, replace string
 	jsonOpts := opts.Clone()
+	jsonOpts.OverrideNullSuffix = true
+
+	var whitespace, replace string
 	if jsonDiff.Action == plans.NoOp && diff.Action == plans.Update {
 		// Then this means we are rendering a whitespace only change. The JSON
 		// differ will have ignored the whitespace changes so that makes the
@@ -214,7 +216,7 @@ func (renderer primitiveRenderer) renderStringDiffAsJson(diff computed.Diff, ind
 	}
 
 	if strings.Contains(renderedJsonDiff, "\n") {
-		return fmt.Sprintf("jsonencode(%s\n%s%s %s%s\n%s)", whitespace, formatIndent(indent+1), colorizeDiffAction(action, opts), renderedJsonDiff, replace, formatIndent(indent+1))
+		return fmt.Sprintf("jsonencode(%s\n%s%s %s%s\n%s)%s", whitespace, formatIndent(indent+1), colorizeDiffAction(action, opts), renderedJsonDiff, replace, formatIndent(indent+1), nullSuffix(diff.Action, opts))
 	}
 	return fmt.Sprintf("jsonencode(%s)%s%s", renderedJsonDiff, whitespace, replace)
 }

internal/command/jsonformat/computed/renderers/renderer_test.go

@@ -137,8 +137,8 @@ jsonencode(
         {
           - key_one = "value_one"
           - key_two = "value_two"
-        } -> null
-    )
+        }
+    ) -> null
 `,
 		},
 		"primitive_json_string_update": {
@@ -153,6 +153,20 @@ jsonencode(
             # (2 unchanged attributes hidden)
         }
     )
+`,
+		},
+		"primitive_json_explicit_nulls": {
+			diff: computed.Diff{
+				Renderer: Primitive("{\"key_one\":\"value_one\",\"key_two\":\"value_two\"}", "{\"key_one\":null}", cty.String),
+				Action:   plans.Update,
+			},
+			expected: `
+jsonencode(
+      ~ {
+          ~ key_one = "value_one" -> null
+          - key_two = "value_two"
+        }
+    )
 `,
 		},
 		"primitive_fake_json_string_update": {

internal/command/jsonformat/computed/renderers/util.go

@@ -55,9 +55,9 @@ func unchanged(keyword string, count int, opts computed.RenderHumanOpts) string
 	return opts.Colorize.Color(fmt.Sprintf("[dark_gray]# (%d unchanged %ss hidden)[reset]", count, keyword))
 }
 
-// ensureValidAttributeName checks if `name` contains any HCL syntax and calls
+// EnsureValidAttributeName checks if `name` contains any HCL syntax and calls
 // and returns hclEscapeString.
-func ensureValidAttributeName(name string) string {
+func EnsureValidAttributeName(name string) string {
 	if !hclsyntax.ValidIdentifier(name) {
 		return hclEscapeString(name)
 	}

internal/command/jsonformat/differ/change_test.go

@@ -35,6 +35,83 @@ func (entry SetDiffEntry) Validate(obj func(attributes map[string]renderers.Vali
 	return obj(entry.ObjectDiff, entry.Action, entry.Replace)
 }
 
+func TestValue_SimpleBlocks(t *testing.T) {
+	// Most of the other test functions wrap the test cases in various
+	// collections or blocks. This function just very simply lets you specify
+	// individual test cases within blocks for some simple tests.
+
+	tcs := map[string]struct {
+		input    Change
+		block    *jsonprovider.Block
+		validate renderers.ValidateDiffFunction
+	}{
+		"delete_with_null_sensitive_value": {
+			input: Change{
+				Before: map[string]interface{}{
+					"normal_attribute": "some value",
+				},
+				After: nil,
+				BeforeSensitive: map[string]interface{}{
+					"sensitive_attribute": true,
+				},
+				AfterSensitive: false,
+			},
+			block: &jsonprovider.Block{
+				Attributes: map[string]*jsonprovider.Attribute{
+					"normal_attribute": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+					"sensitive_attribute": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+				},
+			},
+			validate: renderers.ValidateBlock(map[string]renderers.ValidateDiffFunction{
+				"normal_attribute": renderers.ValidatePrimitive("some value", nil, plans.Delete, false),
+			}, nil, nil, nil, nil, plans.Delete, false),
+		},
+		"create_with_null_sensitive_value": {
+			input: Change{
+				Before: nil,
+				After: map[string]interface{}{
+					"normal_attribute": "some value",
+				},
+				BeforeSensitive: map[string]interface{}{
+					"sensitive_attribute": true,
+				},
+				AfterSensitive: false,
+			},
+			block: &jsonprovider.Block{
+				Attributes: map[string]*jsonprovider.Attribute{
+					"normal_attribute": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+					"sensitive_attribute": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+				},
+			},
+			validate: renderers.ValidateBlock(map[string]renderers.ValidateDiffFunction{
+				"normal_attribute": renderers.ValidatePrimitive(nil, "some value", plans.Create, false),
+			}, nil, nil, nil, nil, plans.Create, false),
+		},
+	}
+	for name, tc := range tcs {
+		// Set some default values
+		if tc.input.ReplacePaths == nil {
+			tc.input.ReplacePaths = &attribute_path.PathMatcher{}
+		}
+
+		if tc.input.RelevantAttributes == nil {
+			tc.input.RelevantAttributes = attribute_path.AlwaysMatcher()
+		}
+
+		t.Run(name, func(t *testing.T) {
+			tc.validate(t, tc.input.ComputeDiffForBlock(tc.block))
+		})
+	}
+}
+
 func TestValue_ObjectAttributes(t *testing.T) {
 	// This function holds a range of test cases creating, deleting and editing
 	// objects. It is built in such a way that it can automatically test these

internal/command/jsonformat/differ/output.go

@@ -17,5 +17,5 @@ func (change Change) ComputeDiffForOutput() computed.Diff {
 	}
 
 	jsonOpts := renderers.RendererJsonOpts()
-	return jsonOpts.Transform(change.Before, change.After, change.RelevantAttributes)
+	return jsonOpts.Transform(change.Before, change.After, change.BeforeExplicit, change.AfterExplicit, change.RelevantAttributes)
 }

internal/command/jsonformat/differ/sensitive.go

@@ -59,7 +59,15 @@ func (change Change) checkForSensitive(create CreateSensitiveRenderer, computedD
 	inner := computedDiff(value)
 
 	action := inner.Action
-	if action == plans.NoOp && beforeSensitive != afterSensitive {
+
+	sensitiveStatusChanged := beforeSensitive != afterSensitive
+
+	// nullNoOp is a stronger NoOp, where not only is there no change happening
+	// but the before and after values are not explicitly set and are both
+	// null. This will override even the sensitive state changing.
+	nullNoOp := change.Before == nil && !change.BeforeExplicit && change.After == nil && !change.AfterExplicit
+
+	if action == plans.NoOp && sensitiveStatusChanged && !nullNoOp {
 		// Let's override this, since it means the sensitive status has changed
 		// rather than the actual content of the value.
 		action = plans.Update

internal/command/jsonformat/jsondiff/diff.go

@@ -3,13 +3,11 @@ package jsondiff
 import (
 	"reflect"
 
-	"github.com/hashicorp/terraform/internal/command/jsonformat/differ/attribute_path"
+	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/command/jsonformat/collections"
 	"github.com/hashicorp/terraform/internal/command/jsonformat/computed"
-
-	"github.com/zclconf/go-cty/cty"
-
+	"github.com/hashicorp/terraform/internal/command/jsonformat/differ/attribute_path"
 	"github.com/hashicorp/terraform/internal/plans"
 )
 
@@ -30,33 +28,36 @@ type JsonOpts struct {
 // Transform accepts a generic before and after value that is assumed to be JSON
 // formatted and transforms it into a computed.Diff, using the callbacks
 // supplied in the JsonOpts class.
-func (opts JsonOpts) Transform(before, after interface{}, relevantAttributes attribute_path.Matcher) computed.Diff {
+func (opts JsonOpts) Transform(before, after interface{}, beforeExplicit, afterExplicit bool, relevantAttributes attribute_path.Matcher) computed.Diff {
 	beforeType := GetType(before)
 	afterType := GetType(after)
 
-	if beforeType == afterType || (beforeType == Null || afterType == Null) {
+	deleted := afterType == Null && !afterExplicit
+	created := beforeType == Null && !beforeExplicit
+
+	if beforeType == afterType || (created || deleted) {
 		targetType := beforeType
 		if targetType == Null {
 			targetType = afterType
 		}
-		return opts.processUpdate(before, after, targetType, relevantAttributes)
+		return opts.processUpdate(before, after, beforeExplicit, afterExplicit, targetType, relevantAttributes)
 	}
 
-	b := opts.processUpdate(before, nil, beforeType, relevantAttributes)
-	a := opts.processUpdate(nil, after, afterType, relevantAttributes)
+	b := opts.processUpdate(before, nil, beforeExplicit, false, beforeType, relevantAttributes)
+	a := opts.processUpdate(nil, after, false, afterExplicit, afterType, relevantAttributes)
 	return opts.TypeChange(b, a, plans.Update)
 }
 
-func (opts JsonOpts) processUpdate(before, after interface{}, jtype Type, relevantAttributes attribute_path.Matcher) computed.Diff {
+func (opts JsonOpts) processUpdate(before, after interface{}, beforeExplicit, afterExplicit bool, jtype Type, relevantAttributes attribute_path.Matcher) computed.Diff {
 	switch jtype {
 	case Null:
-		return opts.processPrimitive(before, after, cty.NilType)
+		return opts.processPrimitive(before, after, beforeExplicit, afterExplicit, cty.NilType)
 	case Bool:
-		return opts.processPrimitive(before, after, cty.Bool)
+		return opts.processPrimitive(before, after, beforeExplicit, afterExplicit, cty.Bool)
 	case String:
-		return opts.processPrimitive(before, after, cty.String)
+		return opts.processPrimitive(before, after, beforeExplicit, afterExplicit, cty.String)
 	case Number:
-		return opts.processPrimitive(before, after, cty.Number)
+		return opts.processPrimitive(before, after, beforeExplicit, afterExplicit, cty.Number)
 	case Object:
 		var b, a map[string]interface{}
 
@@ -86,12 +87,15 @@ func (opts JsonOpts) processUpdate(before, after interface{}, jtype Type, releva
 	}
 }
 
-func (opts JsonOpts) processPrimitive(before, after interface{}, ctype cty.Type) computed.Diff {
+func (opts JsonOpts) processPrimitive(before, after interface{}, beforeExplicit, afterExplicit bool, ctype cty.Type) computed.Diff {
+	beforeMissing := before == nil && !beforeExplicit
+	afterMissing := after == nil && !afterExplicit
+
 	var action plans.Action
 	switch {
-	case before == nil && after != nil:
+	case beforeMissing && !afterMissing:
 		action = plans.Create
-	case before != nil && after == nil:
+	case !beforeMissing && afterMissing:
 		action = plans.Delete
 	case reflect.DeepEqual(before, after):
 		action = plans.NoOp
@@ -106,11 +110,16 @@ func (opts JsonOpts) processArray(before, after []interface{}) computed.Diff {
 	processIndices := func(beforeIx, afterIx int) computed.Diff {
 		var b, a interface{}
 
+		beforeExplicit := false
+		afterExplicit := false
+
 		if beforeIx >= 0 && beforeIx < len(before) {
 			b = before[beforeIx]
+			beforeExplicit = true
 		}
 		if afterIx >= 0 && afterIx < len(after) {
 			a = after[afterIx]
+			afterExplicit = true
 		}
 
 		// It's actually really difficult to render the diffs when some indices
@@ -121,7 +130,7 @@ func (opts JsonOpts) processArray(before, after []interface{}) computed.Diff {
 		// never sets relevant attributes beneath lists or sets. We're just
 		// going to enforce this logic here as well. If the list is relevant
 		// (decided elsewhere), then every element in the list is also relevant.
-		return opts.Transform(b, a, attribute_path.AlwaysMatcher())
+		return opts.Transform(b, a, beforeExplicit, afterExplicit, attribute_path.AlwaysMatcher())
 	}
 
 	isObjType := func(value interface{}) bool {
@@ -133,16 +142,17 @@ func (opts JsonOpts) processArray(before, after []interface{}) computed.Diff {
 
 func (opts JsonOpts) processObject(before, after map[string]interface{}, relevantAttributes attribute_path.Matcher) computed.Diff {
 	return opts.Object(collections.TransformMap(before, after, func(key string) computed.Diff {
+		beforeChild, beforeExplicit := before[key]
+		afterChild, afterExplicit := after[key]
+
 		childRelevantAttributes := relevantAttributes.GetChildWithKey(key)
-
-		beforeChild := before[key]
-		afterChild := after[key]
-
 		if !childRelevantAttributes.MatchesPartial() {
 			// Mark non-relevant attributes as unchanged.
 			afterChild = beforeChild
+			afterExplicit = beforeExplicit
+
 		}
 
-		return opts.Transform(beforeChild, afterChild, childRelevantAttributes)
+		return opts.Transform(beforeChild, afterChild, beforeExplicit, afterExplicit, childRelevantAttributes)
 	}))
 }

internal/command/jsonformat/renderer.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/terraform/internal/command/format"
 	"github.com/hashicorp/terraform/internal/command/jsonformat/computed"
+	"github.com/hashicorp/terraform/internal/command/jsonformat/computed/renderers"
 	"github.com/hashicorp/terraform/internal/command/jsonplan"
 	"github.com/hashicorp/terraform/internal/command/jsonprovider"
 	"github.com/hashicorp/terraform/internal/plans"
@@ -181,7 +182,9 @@ func (r Renderer) RenderHumanPlan(plan Plan, mode plans.Mode, opts ...RendererOp
 	}
 
 	if willPrintResourceChanges {
-		r.Streams.Println("\nTerraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:")
+		r.Streams.Println(format.WordWrap(
+			"\nTerraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:",
+			r.Streams.Stdout.Columns()))
 		if counts[plans.Create] > 0 {
 			r.Streams.Println(r.Colorize.Color(actionDescription(plans.Create)))
 		}
@@ -204,9 +207,9 @@ func (r Renderer) RenderHumanPlan(plan Plan, mode plans.Mode, opts ...RendererOp
 
 	if len(changes) > 0 {
 		if checkOpts(Errored) {
-			r.Streams.Printf("\nTerraform planned the following actions, but then encountered a problem:\n\n")
+			r.Streams.Printf("\nTerraform planned the following actions, but then encountered a problem:\n")
 		} else {
-			r.Streams.Printf("\nTerraform will perform the following actions:\n\n")
+			r.Streams.Printf("\nTerraform will perform the following actions:\n")
 		}
 
 		for _, change := range changes {
@@ -245,15 +248,22 @@ func (r Renderer) renderHumanDiffOutputs(outputs map[string]computed.Diff) strin
 	var rendered []string
 
 	var keys []string
+	escapedKeys := make(map[string]string)
+	var escapedKeyMaxLen int
 	for key := range outputs {
+		escapedKey := renderers.EnsureValidAttributeName(key)
 		keys = append(keys, key)
+		escapedKeys[key] = escapedKey
+		if len(escapedKey) > escapedKeyMaxLen {
+			escapedKeyMaxLen = len(escapedKey)
+		}
 	}
 	sort.Strings(keys)
 
 	for _, key := range keys {
 		output := outputs[key]
 		if output.Action != plans.NoOp {
-			rendered = append(rendered, fmt.Sprintf("%s %s = %s", r.Colorize.Color(format.DiffActionSymbol(output.Action)), key, output.RenderHuman(0, computed.NewRenderHumanOpts(r.Colorize))))
+			rendered = append(rendered, fmt.Sprintf("%s %-*s = %s", r.Colorize.Color(format.DiffActionSymbol(output.Action)), escapedKeyMaxLen, escapedKeys[key], output.RenderHuman(0, computed.NewRenderHumanOpts(r.Colorize))))
 		}
 	}
 	return strings.Join(rendered, "\n")