From 5907a863014e3df46496ccc48859c53d536f6586 Mon Sep 17 00:00:00 2001 From: kmoe <5575356+kmoe@users.noreply.github.com> Date: Fri, 1 Apr 2022 10:09:28 +0100 Subject: [PATCH] command/format: Correctly quote diff object keys (#30766) When rendering a diff, we should quote object attribute names if the string representation is not a valid identifier. While this is not strictly necessary, it makes the diff output more closely resemble the configuration language, which is less confusing. This commit applies to both top-level schema attributes and any object value attributes. We use a simplistic "%q" Go format string to quote the strings, which is not strictly identical to HCL's quoting requirements, but is the pattern used elsewhere in HCL and Terraform. Co-Authored-By: Katy Moe Co-authored-by: Alisdair McDiarmid --- internal/command/format/diff.go | 57 ++++++++++++------ internal/command/format/diff_test.go | 88 ++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 19 deletions(-) diff --git a/internal/command/format/diff.go b/internal/command/format/diff.go index b8018febdc0b..9be2ea3ebd61 100644 --- a/internal/command/format/diff.go +++ b/internal/command/format/diff.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/mitchellh/colorstring" "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" @@ -320,6 +321,7 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff( blankBeforeBlocks := false attrNames := make([]string, 0, len(attrsS)) + displayAttrNames := make(map[string]string, len(attrsS)) attrNameLen := 0 for name := range attrsS { oldVal := ctyGetAttrMaybeNull(old, name) @@ -333,8 +335,9 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff( } attrNames = append(attrNames, name) - if len(name) > attrNameLen { - attrNameLen = len(name) + displayAttrNames[name] = displayAttributeName(name) + if len(displayAttrNames[name]) > attrNameLen { + attrNameLen = len(displayAttrNames[name]) } } sort.Strings(attrNames) @@ -348,7 +351,7 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff( newVal := ctyGetAttrMaybeNull(new, name) result.bodyWritten = true - skipped := p.writeAttrDiff(name, attrS, oldVal, newVal, attrNameLen, indent, path) + skipped := p.writeAttrDiff(displayAttrNames[name], attrS, oldVal, newVal, attrNameLen, indent, path) if skipped { result.skippedAttributes++ } @@ -1110,23 +1113,26 @@ func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, in atys := ty.AttributeTypes() attrNames := make([]string, 0, len(atys)) + displayAttrNames := make(map[string]string, len(atys)) nameLen := 0 for attrName := range atys { attrNames = append(attrNames, attrName) - if len(attrName) > nameLen { - nameLen = len(attrName) + displayAttrNames[attrName] = displayAttributeName(attrName) + if len(displayAttrNames[attrName]) > nameLen { + nameLen = len(displayAttrNames[attrName]) } } sort.Strings(attrNames) for _, attrName := range attrNames { val := val.GetAttr(attrName) + displayAttrName := displayAttrNames[attrName] p.buf.WriteString("\n") p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) - p.buf.WriteString(attrName) - p.buf.WriteString(strings.Repeat(" ", nameLen-len(attrName))) + p.buf.WriteString(displayAttrName) + p.buf.WriteString(strings.Repeat(" ", nameLen-len(displayAttrName))) p.buf.WriteString(" = ") p.writeValue(val, action, indent+4) } @@ -1458,21 +1464,24 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString("\n") var allKeys []string + displayKeys := make(map[string]string) keyLen := 0 for it := old.ElementIterator(); it.Next(); { k, _ := it.Element() keyStr := k.AsString() allKeys = append(allKeys, keyStr) - if len(keyStr) > keyLen { - keyLen = len(keyStr) + displayKeys[keyStr] = displayAttributeName(keyStr) + if len(displayKeys[keyStr]) > keyLen { + keyLen = len(displayKeys[keyStr]) } } for it := new.ElementIterator(); it.Next(); { k, _ := it.Element() keyStr := k.AsString() allKeys = append(allKeys, keyStr) - if len(keyStr) > keyLen { - keyLen = len(keyStr) + displayKeys[keyStr] = displayAttributeName(keyStr) + if len(displayKeys[keyStr]) > keyLen { + keyLen = len(displayKeys[keyStr]) } } @@ -1515,8 +1524,8 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) - p.writeValue(kV, action, indent+4) - p.buf.WriteString(strings.Repeat(" ", keyLen-len(k))) + p.writeValue(cty.StringVal(displayKeys[k]), action, indent+4) + p.buf.WriteString(strings.Repeat(" ", keyLen-len(displayKeys[k]))) p.buf.WriteString(" = ") switch action { case plans.Create, plans.NoOp: @@ -1563,21 +1572,24 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa forcesNewResource := p.pathForcesNewResource(path) var allKeys []string + displayKeys := make(map[string]string) keyLen := 0 for it := old.ElementIterator(); it.Next(); { k, _ := it.Element() keyStr := k.AsString() allKeys = append(allKeys, keyStr) - if len(keyStr) > keyLen { - keyLen = len(keyStr) + displayKeys[keyStr] = displayAttributeName(keyStr) + if len(displayKeys[keyStr]) > keyLen { + keyLen = len(displayKeys[keyStr]) } } for it := new.ElementIterator(); it.Next(); { k, _ := it.Element() keyStr := k.AsString() allKeys = append(allKeys, keyStr) - if len(keyStr) > keyLen { - keyLen = len(keyStr) + displayKeys[keyStr] = displayAttributeName(keyStr) + if len(displayKeys[keyStr]) > keyLen { + keyLen = len(displayKeys[keyStr]) } } @@ -1615,8 +1627,8 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString(strings.Repeat(" ", indent+2)) p.writeActionSymbol(action) - p.buf.WriteString(k) - p.buf.WriteString(strings.Repeat(" ", keyLen-len(k))) + p.buf.WriteString(displayKeys[k]) + p.buf.WriteString(strings.Repeat(" ", keyLen-len(displayKeys[k]))) p.buf.WriteString(" = ") switch action { @@ -2001,3 +2013,10 @@ func (p *blockBodyDiffPrinter) writeSkippedElems(skipped, indent int) { p.buf.WriteString("\n") } } + +func displayAttributeName(name string) string { + if !hclsyntax.ValidIdentifier(name) { + return fmt.Sprintf("%q", name) + } + return name +} diff --git a/internal/command/format/diff_test.go b/internal/command/format/diff_test.go index 6b656f6a3438..4cb2659b24dd 100644 --- a/internal/command/format/diff_test.go +++ b/internal/command/format/diff_test.go @@ -71,6 +71,34 @@ func TestResourceChange_primitiveTypes(t *testing.T) { + resource "test_instance" "example" { + string = "null " } +`, + }, + "creation (object with quoted keys)": { + Action: plans.Create, + Mode: addrs.ManagedResourceMode, + Before: cty.NullVal(cty.EmptyObject), + After: cty.ObjectVal(map[string]cty.Value{ + "object": cty.ObjectVal(map[string]cty.Value{ + "unquoted": cty.StringVal("value"), + "quoted:key": cty.StringVal("some-value"), + }), + }), + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "object": {Type: cty.Object(map[string]cty.Type{ + "unquoted": cty.String, + "quoted:key": cty.String, + }), Optional: true}, + }, + }, + RequiredReplace: cty.NewPathSet(), + ExpectedOutput: ` # test_instance.example will be created + + resource "test_instance" "example" { + + object = { + + "quoted:key" = "some-value" + + unquoted = "value" + } + } `, }, "deletion": { @@ -157,6 +185,35 @@ func TestResourceChange_primitiveTypes(t *testing.T) { ~ ami = "ami-BEFORE" -> "ami-AFTER" id = "i-02ae66f368e8518a9" } +`, + }, + "update with quoted key": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "saml:aud": cty.StringVal("https://example.com/saml"), + "zeta": cty.StringVal("alpha"), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "saml:aud": cty.StringVal("https://saml.example.com"), + "zeta": cty.StringVal("alpha"), + }), + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true, Computed: true}, + "saml:aud": {Type: cty.String, Optional: true}, + "zeta": {Type: cty.String, Optional: true}, + }, + }, + RequiredReplace: cty.NewPathSet(), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + id = "i-02ae66f368e8518a9" + ~ "saml:aud" = "https://example.com/saml" -> "https://saml.example.com" + # (1 unchanged attribute hidden) + } `, }, "string force-new update": { @@ -598,6 +655,37 @@ func TestResourceChange_JSON(t *testing.T) { } ) } +`, + }, + "in-place update of object with quoted keys": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"aaa": "value", "c:c": "old_value"}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"aaa": "value", "b:bb": "new_value"}`), + }), + Schema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true, Computed: true}, + "json_field": {Type: cty.String, Optional: true}, + }, + }, + RequiredReplace: cty.NewPathSet(), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ id = "i-02ae66f368e8518a9" -> (known after apply) + ~ json_field = jsonencode( + ~ { + + "b:bb" = "new_value" + - "c:c" = "old_value" -> null + # (1 unchanged element hidden) + } + ) + } `, }, "in-place update (from empty tuple)": {