Skip to content

Commit

Permalink
command/format: Correctly quote diff object keys (#30766)
Browse files Browse the repository at this point in the history 
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 <katy@katy.moe>

Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
  • Loading branch information
@kmoe @alisdair
kmoe and alisdair committed Apr 1, 2022
1 parent 88c9b90 commit 5907a86
Show file tree
Hide file tree
Showing 2 changed files with 126 additions and 19 deletions.
57 changes: 38 additions & 19 deletions internal/command/format/diff.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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++
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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])
}
}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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])
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
88 changes: 88 additions & 0 deletions internal/command/format/diff_test.go
View file
Expand Up @@ -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": {
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -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)": {
Expand Down

0 comments on commit 5907a86

Please sign in to comment.