From 46ab53d6515b964b3e26d97683061c9bfdb112af Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Mon, 9 Jan 2023 20:22:59 +0100 Subject: [PATCH] Structured Plan Renderer: Escape object and block keys that don't match HCL syntax (#32483) * Escape object and block keys that don't match HCL syntax * address comments --- internal/command/jsonformat/change/block.go | 42 +++++----- internal/command/jsonformat/change/change.go | 11 +++ internal/command/jsonformat/change/object.go | 30 +++---- .../jsonformat/change/renderer_test.go | 82 +++++++++++++++++++ internal/command/jsonformat/differ/tuple.go | 3 +- 5 files changed, 127 insertions(+), 41 deletions(-) diff --git a/internal/command/jsonformat/change/block.go b/internal/command/jsonformat/change/block.go index 0e54183bbe..1ae175b6c2 100644 --- a/internal/command/jsonformat/change/block.go +++ b/internal/command/jsonformat/change/block.go @@ -25,46 +25,44 @@ func importantAttribute(attr string) bool { } func Block(attributes map[string]Change, blocks map[string][]Change) Renderer { - maximumKeyLen := 0 - for key := range attributes { - if len(key) > maximumKeyLen { - maximumKeyLen = len(key) - } - } - return &blockRenderer{ - attributes: attributes, - blocks: blocks, - maximumKeyLen: maximumKeyLen, + attributes: attributes, + blocks: blocks, } } type blockRenderer struct { NoWarningsRenderer - attributes map[string]Change - blocks map[string][]Change - maximumKeyLen int + attributes map[string]Change + blocks map[string][]Change } func (renderer blockRenderer) Render(change Change, indent int, opts RenderOpts) string { unchangedAttributes := 0 unchangedBlocks := 0 + maximumAttributeKeyLen := 0 + var attributeKeys []string + escapedAttributeKeys := make(map[string]string) + for key := range renderer.attributes { + attributeKeys = append(attributeKeys, key) + escapedKey := change.ensureValidAttributeName(key) + escapedAttributeKeys[key] = escapedKey + if maximumAttributeKeyLen < len(escapedKey) { + maximumAttributeKeyLen = len(escapedKey) + } + } + sort.Strings(attributeKeys) + var buf bytes.Buffer buf.WriteString(fmt.Sprintf("{%s\n", change.forcesReplacement())) for _, importantKey := range importantAttributes { if attribute, ok := renderer.attributes[importantKey]; ok { - buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), renderer.maximumKeyLen, importantKey, attribute.Render(indent+1, opts))) + buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), maximumAttributeKeyLen, importantKey, attribute.Render(indent+1, opts))) } } - var attributeKeys []string - for key := range renderer.attributes { - attributeKeys = append(attributeKeys, key) - } - sort.Strings(attributeKeys) - for _, key := range attributeKeys { if importantAttribute(key) { continue @@ -78,7 +76,7 @@ func (renderer blockRenderer) Render(change Change, indent int, opts RenderOpts) for _, warning := range attribute.Warnings(indent + 1) { buf.WriteString(fmt.Sprintf("%s%s\n", change.indent(indent+1), warning)) } - buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), renderer.maximumKeyLen, key, attribute.Render(indent+1, opts))) + buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), maximumAttributeKeyLen, escapedAttributeKeys[key], attribute.Render(indent+1, opts))) } if unchangedAttributes > 0 { @@ -109,7 +107,7 @@ func (renderer blockRenderer) Render(change Change, indent int, opts RenderOpts) for _, warning := range block.Warnings(indent + 1) { buf.WriteString(fmt.Sprintf("%s%s\n", change.indent(indent+1), warning)) } - buf.WriteString(fmt.Sprintf("%s%s %s %s\n", change.indent(indent+1), format.DiffActionSymbol(block.action), key, block.Render(indent+1, opts))) + buf.WriteString(fmt.Sprintf("%s%s %s %s\n", change.indent(indent+1), format.DiffActionSymbol(block.action), change.ensureValidAttributeName(key), block.Render(indent+1, opts))) } } diff --git a/internal/command/jsonformat/change/change.go b/internal/command/jsonformat/change/change.go index 197d1a9c6a..3dc203daf3 100644 --- a/internal/command/jsonformat/change/change.go +++ b/internal/command/jsonformat/change/change.go @@ -4,6 +4,8 @@ import ( "fmt" "strings" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/terraform/internal/plans" ) @@ -105,3 +107,12 @@ func (change Change) unchanged(keyword string, count int) string { } return fmt.Sprintf("[dark_gray]# (%d unchanged %ss hidden)[reset]", count, keyword) } + +// ensureValidAttributeName checks if `name` contains any HCL syntax and returns +// it surrounded by quotation marks if it does. +func (change Change) ensureValidAttributeName(name string) string { + if !hclsyntax.ValidIdentifier(name) { + return fmt.Sprintf("%q", name) + } + return name +} diff --git a/internal/command/jsonformat/change/object.go b/internal/command/jsonformat/change/object.go index f4814a692d..7d948cd8f7 100644 --- a/internal/command/jsonformat/change/object.go +++ b/internal/command/jsonformat/change/object.go @@ -10,31 +10,15 @@ import ( ) func Object(attributes map[string]Change) Renderer { - maximumKeyLen := 0 - for key := range attributes { - if maximumKeyLen < len(key) { - maximumKeyLen = len(key) - } - } - return &objectRenderer{ attributes: attributes, - maximumKeyLen: maximumKeyLen, overrideNullSuffix: true, } } func NestedObject(attributes map[string]Change) Renderer { - maximumKeyLen := 0 - for key := range attributes { - if maximumKeyLen < len(key) { - maximumKeyLen = len(key) - } - } - return &objectRenderer{ attributes: attributes, - maximumKeyLen: maximumKeyLen, overrideNullSuffix: false, } } @@ -43,7 +27,6 @@ type objectRenderer struct { NoWarningsRenderer attributes map[string]Change - maximumKeyLen int overrideNullSuffix bool } @@ -55,9 +38,20 @@ func (renderer objectRenderer) Render(change Change, indent int, opts RenderOpts attributeOpts := opts.Clone() attributeOpts.overrideNullSuffix = renderer.overrideNullSuffix + // We need to keep track of our keys in two ways. The first is the order in + // which we will display them. The second is a mapping to their safely + // escaped equivalent. + + maximumKeyLen := 0 var keys []string + escapedKeys := make(map[string]string) for key := range renderer.attributes { keys = append(keys, key) + escapedKey := change.ensureValidAttributeName(key) + escapedKeys[key] = escapedKey + if maximumKeyLen < len(escapedKey) { + maximumKeyLen = len(escapedKey) + } } sort.Strings(keys) @@ -76,7 +70,7 @@ func (renderer objectRenderer) Render(change Change, indent int, opts RenderOpts for _, warning := range attribute.Warnings(indent + 1) { buf.WriteString(fmt.Sprintf("%s%s\n", change.indent(indent+1), warning)) } - buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), renderer.maximumKeyLen, key, attribute.Render(indent+1, attributeOpts))) + buf.WriteString(fmt.Sprintf("%s%s %-*s = %s\n", change.indent(indent+1), format.DiffActionSymbol(attribute.action), maximumKeyLen, escapedKeys[key], attribute.Render(indent+1, attributeOpts))) } if unchangedAttributes > 0 { diff --git a/internal/command/jsonformat/change/renderer_test.go b/internal/command/jsonformat/change/renderer_test.go index 2596a9fb7e..fa1f639a44 100644 --- a/internal/command/jsonformat/change/renderer_test.go +++ b/internal/command/jsonformat/change/renderer_test.go @@ -349,6 +349,32 @@ func TestRenderers(t *testing.T) { { ~ attribute_one = 1 -> (known after apply) } +`, + }, + "object_escapes_attribute_keys": { + change: Change{ + renderer: Object(map[string]Change{ + "attribute_one": { + renderer: Primitive(strptr("1"), strptr("2")), + action: plans.Update, + }, + "attribute:two": { + renderer: Primitive(strptr("2"), strptr("3")), + action: plans.Update, + }, + "attribute_six": { + renderer: Primitive(strptr("3"), strptr("4")), + action: plans.Update, + }, + }), + action: plans.Update, + }, + expected: ` +{ + ~ "attribute:two" = 2 -> 3 + ~ attribute_one = 1 -> 2 + ~ attribute_six = 3 -> 4 + } `, }, "map_create_empty": { @@ -1425,6 +1451,62 @@ func TestRenderers(t *testing.T) { }, expected: ` { + }`, + }, + "block_escapes_keys": { + change: Change{ + renderer: Block(map[string]Change{ + "attribute_one": { + renderer: Primitive(strptr("1"), strptr("2")), + action: plans.Update, + }, + "attribute:two": { + renderer: Primitive(strptr("2"), strptr("3")), + action: plans.Update, + }, + "attribute_six": { + renderer: Primitive(strptr("3"), strptr("4")), + action: plans.Update, + }, + }, map[string][]Change{ + "nested_block:one": { + { + renderer: Block(map[string]Change{ + "string": { + renderer: Primitive(strptr("\"one\""), strptr("\"four\"")), + action: plans.Update, + }, + }, nil), + action: plans.Update, + }, + }, + "nested_block_two": { + { + renderer: Block(map[string]Change{ + "string": { + renderer: Primitive(strptr("\"two\""), strptr("\"three\"")), + action: plans.Update, + }, + }, nil), + action: plans.Update, + }, + }, + }), + action: plans.Update, + }, + expected: ` +{ + ~ "attribute:two" = 2 -> 3 + ~ attribute_one = 1 -> 2 + ~ attribute_six = 3 -> 4 + + ~ "nested_block:one" { + ~ string = "one" -> "four" + } + + ~ nested_block_two { + ~ string = "two" -> "three" + } }`, }, "block_always_includes_important_attributes": { diff --git a/internal/command/jsonformat/differ/tuple.go b/internal/command/jsonformat/differ/tuple.go index 6f9b117ce7..fadbbc7b21 100644 --- a/internal/command/jsonformat/differ/tuple.go +++ b/internal/command/jsonformat/differ/tuple.go @@ -1,8 +1,9 @@ package differ import ( - "github.com/hashicorp/terraform/internal/command/jsonformat/change" "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/command/jsonformat/change" ) func (v Value) computeAttributeChangeAsTuple(elementTypes []cty.Type) change.Change {