Skip to content

Commit 5907a86

Browse files
kmoealisdair
kmoe
and
alisdair
authored
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 <katy@katy.moe> Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
1 parent 88c9b90 commit 5907a86

File tree

2 files changed

+126
-19
lines changed

2 files changed

+126
-19
lines changed

‎internal/command/format/diff.go‎

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"sort"
99
"strings"
1010

11+
"github.com/hashicorp/hcl/v2/hclsyntax"
1112
"github.com/mitchellh/colorstring"
1213
"github.com/zclconf/go-cty/cty"
1314
ctyjson "github.com/zclconf/go-cty/cty/json"
@@ -320,6 +321,7 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff(
320321
blankBeforeBlocks := false
321322

322323
attrNames := make([]string, 0, len(attrsS))
324+
displayAttrNames := make(map[string]string, len(attrsS))
323325
attrNameLen := 0
324326
for name := range attrsS {
325327
oldVal := ctyGetAttrMaybeNull(old, name)
@@ -333,8 +335,9 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff(
333335
}
334336

335337
attrNames = append(attrNames, name)
336-
if len(name) > attrNameLen {
337-
attrNameLen = len(name)
338+
displayAttrNames[name] = displayAttributeName(name)
339+
if len(displayAttrNames[name]) > attrNameLen {
340+
attrNameLen = len(displayAttrNames[name])
338341
}
339342
}
340343
sort.Strings(attrNames)
@@ -348,7 +351,7 @@ func (p *blockBodyDiffPrinter) writeAttrsDiff(
348351
newVal := ctyGetAttrMaybeNull(new, name)
349352

350353
result.bodyWritten = true
351-
skipped := p.writeAttrDiff(name, attrS, oldVal, newVal, attrNameLen, indent, path)
354+
skipped := p.writeAttrDiff(displayAttrNames[name], attrS, oldVal, newVal, attrNameLen, indent, path)
352355
if skipped {
353356
result.skippedAttributes++
354357
}
@@ -1110,23 +1113,26 @@ func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, in
11101113

11111114
atys := ty.AttributeTypes()
11121115
attrNames := make([]string, 0, len(atys))
1116+
displayAttrNames := make(map[string]string, len(atys))
11131117
nameLen := 0
11141118
for attrName := range atys {
11151119
attrNames = append(attrNames, attrName)
1116-
if len(attrName) > nameLen {
1117-
nameLen = len(attrName)
1120+
displayAttrNames[attrName] = displayAttributeName(attrName)
1121+
if len(displayAttrNames[attrName]) > nameLen {
1122+
nameLen = len(displayAttrNames[attrName])
11181123
}
11191124
}
11201125
sort.Strings(attrNames)
11211126

11221127
for _, attrName := range attrNames {
11231128
val := val.GetAttr(attrName)
1129+
displayAttrName := displayAttrNames[attrName]
11241130

11251131
p.buf.WriteString("\n")
11261132
p.buf.WriteString(strings.Repeat(" ", indent+2))
11271133
p.writeActionSymbol(action)
1128-
p.buf.WriteString(attrName)
1129-
p.buf.WriteString(strings.Repeat(" ", nameLen-len(attrName)))
1134+
p.buf.WriteString(displayAttrName)
1135+
p.buf.WriteString(strings.Repeat(" ", nameLen-len(displayAttrName)))
11301136
p.buf.WriteString(" = ")
11311137
p.writeValue(val, action, indent+4)
11321138
}
@@ -1458,21 +1464,24 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa
14581464
p.buf.WriteString("\n")
14591465

14601466
var allKeys []string
1467+
displayKeys := make(map[string]string)
14611468
keyLen := 0
14621469
for it := old.ElementIterator(); it.Next(); {
14631470
k, _ := it.Element()
14641471
keyStr := k.AsString()
14651472
allKeys = append(allKeys, keyStr)
1466-
if len(keyStr) > keyLen {
1467-
keyLen = len(keyStr)
1473+
displayKeys[keyStr] = displayAttributeName(keyStr)
1474+
if len(displayKeys[keyStr]) > keyLen {
1475+
keyLen = len(displayKeys[keyStr])
14681476
}
14691477
}
14701478
for it := new.ElementIterator(); it.Next(); {
14711479
k, _ := it.Element()
14721480
keyStr := k.AsString()
14731481
allKeys = append(allKeys, keyStr)
1474-
if len(keyStr) > keyLen {
1475-
keyLen = len(keyStr)
1482+
displayKeys[keyStr] = displayAttributeName(keyStr)
1483+
if len(displayKeys[keyStr]) > keyLen {
1484+
keyLen = len(displayKeys[keyStr])
14761485
}
14771486
}
14781487

@@ -1515,8 +1524,8 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa
15151524

15161525
p.buf.WriteString(strings.Repeat(" ", indent+2))
15171526
p.writeActionSymbol(action)
1518-
p.writeValue(kV, action, indent+4)
1519-
p.buf.WriteString(strings.Repeat(" ", keyLen-len(k)))
1527+
p.writeValue(cty.StringVal(displayKeys[k]), action, indent+4)
1528+
p.buf.WriteString(strings.Repeat(" ", keyLen-len(displayKeys[k])))
15201529
p.buf.WriteString(" = ")
15211530
switch action {
15221531
case plans.Create, plans.NoOp:
@@ -1563,21 +1572,24 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa
15631572
forcesNewResource := p.pathForcesNewResource(path)
15641573

15651574
var allKeys []string
1575+
displayKeys := make(map[string]string)
15661576
keyLen := 0
15671577
for it := old.ElementIterator(); it.Next(); {
15681578
k, _ := it.Element()
15691579
keyStr := k.AsString()
15701580
allKeys = append(allKeys, keyStr)
1571-
if len(keyStr) > keyLen {
1572-
keyLen = len(keyStr)
1581+
displayKeys[keyStr] = displayAttributeName(keyStr)
1582+
if len(displayKeys[keyStr]) > keyLen {
1583+
keyLen = len(displayKeys[keyStr])
15731584
}
15741585
}
15751586
for it := new.ElementIterator(); it.Next(); {
15761587
k, _ := it.Element()
15771588
keyStr := k.AsString()
15781589
allKeys = append(allKeys, keyStr)
1579-
if len(keyStr) > keyLen {
1580-
keyLen = len(keyStr)
1590+
displayKeys[keyStr] = displayAttributeName(keyStr)
1591+
if len(displayKeys[keyStr]) > keyLen {
1592+
keyLen = len(displayKeys[keyStr])
15811593
}
15821594
}
15831595

@@ -1615,8 +1627,8 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa
16151627

16161628
p.buf.WriteString(strings.Repeat(" ", indent+2))
16171629
p.writeActionSymbol(action)
1618-
p.buf.WriteString(k)
1619-
p.buf.WriteString(strings.Repeat(" ", keyLen-len(k)))
1630+
p.buf.WriteString(displayKeys[k])
1631+
p.buf.WriteString(strings.Repeat(" ", keyLen-len(displayKeys[k])))
16201632
p.buf.WriteString(" = ")
16211633

16221634
switch action {
@@ -2001,3 +2013,10 @@ func (p *blockBodyDiffPrinter) writeSkippedElems(skipped, indent int) {
20012013
p.buf.WriteString("\n")
20022014
}
20032015
}
2016+
2017+
func displayAttributeName(name string) string {
2018+
if !hclsyntax.ValidIdentifier(name) {
2019+
return fmt.Sprintf("%q", name)
2020+
}
2021+
return name
2022+
}

‎internal/command/format/diff_test.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,34 @@ func TestResourceChange_primitiveTypes(t *testing.T) {
7171
+ resource "test_instance" "example" {
7272
+ string = "null "
7373
}
74+
`,
75+
},
76+
"creation (object with quoted keys)": {
77+
Action: plans.Create,
78+
Mode: addrs.ManagedResourceMode,
79+
Before: cty.NullVal(cty.EmptyObject),
80+
After: cty.ObjectVal(map[string]cty.Value{
81+
"object": cty.ObjectVal(map[string]cty.Value{
82+
"unquoted": cty.StringVal("value"),
83+
"quoted:key": cty.StringVal("some-value"),
84+
}),
85+
}),
86+
Schema: &configschema.Block{
87+
Attributes: map[string]*configschema.Attribute{
88+
"object": {Type: cty.Object(map[string]cty.Type{
89+
"unquoted": cty.String,
90+
"quoted:key": cty.String,
91+
}), Optional: true},
92+
},
93+
},
94+
RequiredReplace: cty.NewPathSet(),
95+
ExpectedOutput: ` # test_instance.example will be created
96+
+ resource "test_instance" "example" {
97+
+ object = {
98+
+ "quoted:key" = "some-value"
99+
+ unquoted = "value"
100+
}
101+
}
74102
`,
75103
},
76104
"deletion": {
@@ -157,6 +185,35 @@ func TestResourceChange_primitiveTypes(t *testing.T) {
157185
~ ami = "ami-BEFORE" -> "ami-AFTER"
158186
id = "i-02ae66f368e8518a9"
159187
}
188+
`,
189+
},
190+
"update with quoted key": {
191+
Action: plans.Update,
192+
Mode: addrs.ManagedResourceMode,
193+
Before: cty.ObjectVal(map[string]cty.Value{
194+
"id": cty.StringVal("i-02ae66f368e8518a9"),
195+
"saml:aud": cty.StringVal("https://example.com/saml"),
196+
"zeta": cty.StringVal("alpha"),
197+
}),
198+
After: cty.ObjectVal(map[string]cty.Value{
199+
"id": cty.StringVal("i-02ae66f368e8518a9"),
200+
"saml:aud": cty.StringVal("https://saml.example.com"),
201+
"zeta": cty.StringVal("alpha"),
202+
}),
203+
Schema: &configschema.Block{
204+
Attributes: map[string]*configschema.Attribute{
205+
"id": {Type: cty.String, Optional: true, Computed: true},
206+
"saml:aud": {Type: cty.String, Optional: true},
207+
"zeta": {Type: cty.String, Optional: true},
208+
},
209+
},
210+
RequiredReplace: cty.NewPathSet(),
211+
ExpectedOutput: ` # test_instance.example will be updated in-place
212+
~ resource "test_instance" "example" {
213+
id = "i-02ae66f368e8518a9"
214+
~ "saml:aud" = "https://example.com/saml" -> "https://saml.example.com"
215+
# (1 unchanged attribute hidden)
216+
}
160217
`,
161218
},
162219
"string force-new update": {
@@ -598,6 +655,37 @@ func TestResourceChange_JSON(t *testing.T) {
598655
}
599656
)
600657
}
658+
`,
659+
},
660+
"in-place update of object with quoted keys": {
661+
Action: plans.Update,
662+
Mode: addrs.ManagedResourceMode,
663+
Before: cty.ObjectVal(map[string]cty.Value{
664+
"id": cty.StringVal("i-02ae66f368e8518a9"),
665+
"json_field": cty.StringVal(`{"aaa": "value", "c:c": "old_value"}`),
666+
}),
667+
After: cty.ObjectVal(map[string]cty.Value{
668+
"id": cty.UnknownVal(cty.String),
669+
"json_field": cty.StringVal(`{"aaa": "value", "b:bb": "new_value"}`),
670+
}),
671+
Schema: &configschema.Block{
672+
Attributes: map[string]*configschema.Attribute{
673+
"id": {Type: cty.String, Optional: true, Computed: true},
674+
"json_field": {Type: cty.String, Optional: true},
675+
},
676+
},
677+
RequiredReplace: cty.NewPathSet(),
678+
ExpectedOutput: ` # test_instance.example will be updated in-place
679+
~ resource "test_instance" "example" {
680+
~ id = "i-02ae66f368e8518a9" -> (known after apply)
681+
~ json_field = jsonencode(
682+
~ {
683+
+ "b:bb" = "new_value"
684+
- "c:c" = "old_value" -> null
685+
# (1 unchanged element hidden)
686+
}
687+
)
688+
}
601689
`,
602690
},
603691
"in-place update (from empty tuple)": {

0 commit comments

Comments
 (0)