From 2c624acea19ea8eb884af7cbe27cf65e488ad0a2 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Fri, 14 Apr 2023 09:56:32 +0200 Subject: [PATCH] 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 --- .../computed/renderers/primitive.go | 11 ++- .../computed/renderers/renderer_test.go | 97 +++++++++++++++++++ .../jsonformat/computed/renderers/string.go | 14 ++- .../command/jsonformat/differ/change_test.go | 89 +++++++++++++++++ internal/command/jsonformat/differ/map.go | 34 ++++++- 5 files changed, 240 insertions(+), 5 deletions(-) diff --git a/internal/command/jsonformat/computed/renderers/primitive.go b/internal/command/jsonformat/computed/renderers/primitive.go index a7a6dc5932..9891945c8b 100644 --- a/internal/command/jsonformat/computed/renderers/primitive.go +++ b/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") diff --git a/internal/command/jsonformat/computed/renderers/renderer_test.go b/internal/command/jsonformat/computed/renderers/renderer_test.go index 352e3dd523..94a3694a21 100644 --- a/internal/command/jsonformat/computed/renderers/renderer_test.go +++ b/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), diff --git a/internal/command/jsonformat/computed/renderers/string.go b/internal/command/jsonformat/computed/renderers/string.go index 91fbd62059..7777a3eb42 100644 --- a/internal/command/jsonformat/computed/renderers/string.go +++ b/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) +} diff --git a/internal/command/jsonformat/differ/change_test.go b/internal/command/jsonformat/differ/change_test.go index 3ac50ce88b..bccbf7b5db 100644 --- a/internal/command/jsonformat/differ/change_test.go +++ b/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. diff --git a/internal/command/jsonformat/differ/map.go b/internal/command/jsonformat/differ/map.go index e33b6dcdcf..6f92622e63 100644 --- a/internal/command/jsonformat/differ/map.go +++ b/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.