Fix rendering unknown values in map and null string primitives (#33029)

* fix rendering unknown values in map and null string primitives

* Update map.go

* fix code consistency checks

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

@@ -88,7 +88,7 @@ func (renderer primitiveRenderer) renderStringDiff(diff computed.Diff, indent in
 		}
 
 		if !str.IsMultiline {
-			return fmt.Sprintf("%q%s", str.String, forcesReplacement(diff.Replace, opts))
+			return fmt.Sprintf("%s%s", str.RenderSimple(), forcesReplacement(diff.Replace, opts))
 		}
 
 		// We are creating a single multiline string, so let's split by the new
@@ -102,13 +102,18 @@ func (renderer primitiveRenderer) renderStringDiff(diff computed.Diff, indent in
 		lines[0] = fmt.Sprintf("%s%s%s", formatIndent(indent+1), writeDiffActionSymbol(plans.NoOp, opts), lines[0])
 	case plans.Delete:
 		str := evaluatePrimitiveString(renderer.before, opts)
+		if str.IsNull {
+			// We don't put the null suffix (-> null) here because the final
+			// render or null -> null would look silly.
+			return fmt.Sprintf("%s%s", str.RenderSimple(), forcesReplacement(diff.Replace, opts))
+		}
 
 		if str.Json != nil {
 			return renderer.renderStringDiffAsJson(diff, indent, opts, str, evaluatedString{})
 		}
 
 		if !str.IsMultiline {
-			return fmt.Sprintf("%q%s%s", str.String, nullSuffix(diff.Action, opts), forcesReplacement(diff.Replace, opts))
+			return fmt.Sprintf("%s%s%s", str.RenderSimple(), nullSuffix(diff.Action, opts), forcesReplacement(diff.Replace, opts))
 		}
 
 		// We are creating a single multiline string, so let's split by the new
@@ -141,7 +146,7 @@ func (renderer primitiveRenderer) renderStringDiff(diff computed.Diff, indent in
 		}
 
 		if !beforeString.IsMultiline && !afterString.IsMultiline {
-			return fmt.Sprintf("%q %s %q%s", beforeString.String, opts.Colorize.Color("[yellow]->[reset]"), afterString.String, forcesReplacement(diff.Replace, opts))
+			return fmt.Sprintf("%s %s %s%s", beforeString.RenderSimple(), opts.Colorize.Color("[yellow]->[reset]"), afterString.RenderSimple(), forcesReplacement(diff.Replace, opts))
 		}
 
 		beforeLines := strings.Split(beforeString.String, "\n")

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

@@ -24,6 +24,103 @@ func TestRenderers_Human(t *testing.T) {
 		expected string
 		opts     computed.RenderHumanOpts
 	}{
+		// We're using the string "null" in these tests to demonstrate the
+		// difference between rendering an actual string and rendering a null
+		// value.
+		"primitive_create_string": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, "null", cty.String),
+				Action:   plans.Create,
+			},
+			expected: "\"null\"",
+		},
+		"primitive_delete_string": {
+			diff: computed.Diff{
+				Renderer: Primitive("null", nil, cty.String),
+				Action:   plans.Delete,
+			},
+			expected: "\"null\" -> null",
+		},
+		"primitive_update_string_to_null": {
+			diff: computed.Diff{
+				Renderer: Primitive("null", nil, cty.String),
+				Action:   plans.Update,
+			},
+			expected: "\"null\" -> null",
+		},
+		"primitive_update_string_from_null": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, "null", cty.String),
+				Action:   plans.Update,
+			},
+			expected: "null -> \"null\"",
+		},
+		"primitive_update_multiline_string_to_null": {
+			diff: computed.Diff{
+				Renderer: Primitive("nu\nll", nil, cty.String),
+				Action:   plans.Update,
+			},
+			expected: `
+<<-EOT
+      - nu
+      - ll
+      + null
+    EOT
+`,
+		},
+		"primitive_update_multiline_string_from_null": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, "nu\nll", cty.String),
+				Action:   plans.Update,
+			},
+			expected: `
+<<-EOT
+      - null
+      + nu
+      + ll
+    EOT
+`,
+		},
+		"primitive_update_json_string_to_null": {
+			diff: computed.Diff{
+				Renderer: Primitive("[null]", nil, cty.String),
+				Action:   plans.Update,
+			},
+			expected: `
+jsonencode(
+        [
+          - null,
+        ]
+    ) -> null
+`,
+		},
+		"primitive_update_json_string_from_null": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, "[null]", cty.String),
+				Action:   plans.Update,
+			},
+			expected: `
+null -> jsonencode(
+        [
+          + null,
+        ]
+    )
+`,
+		},
+		"primitive_create_null_string": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, nil, cty.String),
+				Action:   plans.Create,
+			},
+			expected: "null",
+		},
+		"primitive_delete_null_string": {
+			diff: computed.Diff{
+				Renderer: Primitive(nil, nil, cty.String),
+				Action:   plans.Delete,
+			},
+			expected: "null",
+		},
 		"primitive_create": {
 			diff: computed.Diff{
 				Renderer: Primitive(nil, 1.0, cty.Number),

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

@@ -2,6 +2,7 @@ package renderers
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/hashicorp/terraform/internal/command/jsonformat/computed"
@@ -12,11 +13,15 @@ type evaluatedString struct {
 	Json   interface{}
 
 	IsMultiline bool
+	IsNull      bool
 }
 
 func evaluatePrimitiveString(value interface{}, opts computed.RenderHumanOpts) evaluatedString {
 	if value == nil {
-		return evaluatedString{String: opts.Colorize.Color("[dark_gray]null[reset]")}
+		return evaluatedString{
+			String: opts.Colorize.Color("[dark_gray]null[reset]"),
+			IsNull: true,
+		}
 	}
 
 	str := value.(string)
@@ -42,3 +47,10 @@ func evaluatePrimitiveString(value interface{}, opts computed.RenderHumanOpts) e
 		String: str,
 	}
 }
+
+func (e evaluatedString) RenderSimple() string {
+	if e.IsNull {
+		return e.String
+	}
+	return fmt.Sprintf("%q", e.String)
+}

internal/command/jsonformat/differ/change_test.go

@@ -2552,6 +2552,95 @@ func TestRelevantAttributes(t *testing.T) {
 	}
 }
 
+func TestSpecificCases(t *testing.T) {
+	// This is a special test that can contain any combination of individual
+	// cases and will execute against them. For testing/fixing specific issues
+	// you can generally put the test case in here.
+	tcs := map[string]struct {
+		input    Change
+		block    *jsonprovider.Block
+		validate renderers.ValidateDiffFunction
+	}{
+		"issues/33016/unknown": {
+			input: Change{
+				Before: nil,
+				After: map[string]interface{}{
+					"triggers": map[string]interface{}{},
+				},
+				Unknown: map[string]interface{}{
+					"id": true,
+					"triggers": map[string]interface{}{
+						"rotation": true,
+					},
+				},
+				BeforeSensitive: false,
+				AfterSensitive: map[string]interface{}{
+					"triggers": map[string]interface{}{},
+				},
+				ReplacePaths:       attribute_path.Empty(false),
+				RelevantAttributes: attribute_path.AlwaysMatcher(),
+			},
+			block: &jsonprovider.Block{
+				Attributes: map[string]*jsonprovider.Attribute{
+					"id": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+					"triggers": {
+						AttributeType: unmarshalType(t, cty.Map(cty.String)),
+					},
+				},
+			},
+			validate: renderers.ValidateBlock(map[string]renderers.ValidateDiffFunction{
+				"id": renderers.ValidateUnknown(nil, plans.Create, false),
+				"triggers": renderers.ValidateMap(map[string]renderers.ValidateDiffFunction{
+					"rotation": renderers.ValidateUnknown(nil, plans.Create, false),
+				}, plans.Create, false),
+			}, nil, nil, nil, nil, plans.Create, false),
+		},
+		"issues/33016/null": {
+			input: Change{
+				Before: nil,
+				After: map[string]interface{}{
+					"triggers": map[string]interface{}{
+						"rotation": nil,
+					},
+				},
+				Unknown: map[string]interface{}{
+					"id":       true,
+					"triggers": map[string]interface{}{},
+				},
+				BeforeSensitive: false,
+				AfterSensitive: map[string]interface{}{
+					"triggers": map[string]interface{}{},
+				},
+				ReplacePaths:       attribute_path.Empty(false),
+				RelevantAttributes: attribute_path.AlwaysMatcher(),
+			},
+			block: &jsonprovider.Block{
+				Attributes: map[string]*jsonprovider.Attribute{
+					"id": {
+						AttributeType: unmarshalType(t, cty.String),
+					},
+					"triggers": {
+						AttributeType: unmarshalType(t, cty.Map(cty.String)),
+					},
+				},
+			},
+			validate: renderers.ValidateBlock(map[string]renderers.ValidateDiffFunction{
+				"id": renderers.ValidateUnknown(nil, plans.Create, false),
+				"triggers": renderers.ValidateMap(map[string]renderers.ValidateDiffFunction{
+					"rotation": renderers.ValidatePrimitive(nil, nil, plans.Create, false),
+				}, plans.Create, false),
+			}, nil, nil, nil, nil, plans.Create, false),
+		},
+	}
+	for name, tc := range tcs {
+		t.Run(name, func(t *testing.T) {
+			tc.validate(t, tc.input.ComputeDiffForBlock(tc.block))
+		})
+	}
+}
+
 // unmarshalType converts a cty.Type into a json.RawMessage understood by the
 // schema. It also lets the testing framework handle any errors to keep the API
 // clean.

internal/command/jsonformat/differ/map.go

@@ -12,7 +12,39 @@ import (
 
 func (change Change) computeAttributeDiffAsMap(elementType cty.Type) computed.Diff {
 	mapValue := change.asMap()
-	elements, current := collections.TransformMap(mapValue.Before, mapValue.After, func(key string) computed.Diff {
+
+	// The jsonplan package will have stripped out unknowns from our after value
+	// so we're going to add them back in here.
+	//
+	// This only affects attributes and not nested attributes or blocks, so we
+	// only perform this fix in this function and not the equivalent map
+	// functions for nested attributes and blocks.
+
+	// There is actually a difference between a null map and an empty map for
+	// purposes of calculating a delete, create, or update operation.
+
+	var after map[string]interface{}
+	if mapValue.After != nil {
+		after = make(map[string]interface{})
+	}
+
+	for key, value := range mapValue.After {
+		after[key] = value
+	}
+	for key := range mapValue.Unknown {
+		if _, ok := after[key]; ok {
+			// Then this unknown value was in after, this probably means it has
+			// a child that is unknown rather than being unknown itself. As
+			// such, we'll skip over it. Note, it doesn't particularly matter if
+			// an element is in both places - it's just important we actually
+			// do cover all the elements. We want a complete union and therefore
+			// duplicates are no cause for concern as long as we dedupe here.
+			continue
+		}
+		after[key] = nil
+	}
+
+	elements, current := collections.TransformMap(mapValue.Before, after, func(key string) computed.Diff {
 		value := mapValue.getChild(key)
 		if !value.RelevantAttributes.MatchesPartial() {
 			// Mark non-relevant attributes as unchanged.