From 3d0a25c65df992544ee6f6b1d293b8f52f35b0a1 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Tue, 15 Jan 2019 15:37:03 +0000 Subject: [PATCH 1/4] command/format: Fix nested (JSON) object formatting --- command/format/diff.go | 123 +++++++++++++-- command/format/diff_test.go | 305 +++++++++++++++++++++++++++++++++++- 2 files changed, 407 insertions(+), 21 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index d88a80147..24026db0e 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -570,8 +570,6 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // values are known and non-null. if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() { switch { - // TODO: object diffs that behave a bit like the map diffs, including if the two object types don't exactly match - case ty == cty.String: // We have special behavior for both multi-line strings in general // and for strings that can parse as JSON. For the JSON handling @@ -747,7 +745,6 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString("]") return - case ty.IsListType() || ty.IsTupleType(): p.buf.WriteString("[") if p.pathForcesNewResource(path) { @@ -755,12 +752,27 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa } p.buf.WriteString("\n") - elemDiffs := ctySequenceDiff(old.AsValueSlice(), new.AsValueSlice()) - for _, elemDiff := range elemDiffs { - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.writeActionSymbol(elemDiff.Action) - p.writeValue(elemDiff.Value, elemDiff.Action, indent+4) - p.buf.WriteString(",\n") + if ty.IsTupleType() && ty.Length() > 0 && ty.TupleElementType(0).IsObjectType() { + elemDiffs := ctyObjectSequenceDiff(old.AsValueSlice(), new.AsValueSlice(), 0) + for _, elemDiff := range elemDiffs { + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.writeActionSymbol(elemDiff.Action) + if elemDiff.Action == plans.NoOp { + p.writeValue(elemDiff.Before, elemDiff.Action, indent+4) + } else { + p.writeValueDiff(elemDiff.Before, elemDiff.After, indent+4, path) + } + + p.buf.WriteString(",\n") + } + } else { + elemDiffs := ctySequenceDiff(old.AsValueSlice(), new.AsValueSlice()) + for _, elemDiff := range elemDiffs { + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.writeActionSymbol(elemDiff.Action) + p.writeValue(elemDiff.Value, elemDiff.Action, indent+4) + p.buf.WriteString(",\n") + } } p.buf.WriteString(strings.Repeat(" ", indent)) @@ -841,15 +853,96 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa p.buf.WriteString(strings.Repeat(" ", indent)) p.buf.WriteString("}") return + case ty.IsObjectType(): + p.buf.WriteString("{") + p.buf.WriteString("\n") + + forcesNewResource := p.pathForcesNewResource(path) + + var allKeys []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) + } + } + for it := new.ElementIterator(); it.Next(); { + k, _ := it.Element() + keyStr := k.AsString() + allKeys = append(allKeys, keyStr) + if len(keyStr) > keyLen { + keyLen = len(keyStr) + } + } + + sort.Strings(allKeys) + + lastK := "" + for i, k := range allKeys { + if i > 0 && lastK == k { + continue // skip duplicates (list is sorted) + } + lastK = k + + p.buf.WriteString(strings.Repeat(" ", indent+2)) + kV := k + var action plans.Action + if !old.Type().HasAttribute(kV) { + action = plans.Create + } else if !new.Type().HasAttribute(kV) { + action = plans.Delete + } else if eqV := old.GetAttr(kV).Equals(new.GetAttr(kV)); eqV.IsKnown() && eqV.True() { + action = plans.NoOp + } else { + action = plans.Update + } + + path := append(path, cty.GetAttrStep{Name: kV}) + + p.writeActionSymbol(action) + p.buf.WriteString(k) + p.buf.WriteString(strings.Repeat(" ", keyLen-len(k))) + p.buf.WriteString(" = ") + + switch action { + case plans.Create, plans.NoOp: + v := new.GetAttr(kV) + p.writeValue(v, action, indent+4) + case plans.Delete: + oldV := old.GetAttr(kV) + newV := cty.NullVal(oldV.Type()) + p.writeValueDiff(oldV, newV, indent+4, path) + default: + oldV := old.GetAttr(kV) + newV := new.GetAttr(kV) + p.writeValueDiff(oldV, newV, indent+4, path) + } + + p.buf.WriteString("\n") + } + + p.buf.WriteString(strings.Repeat(" ", indent)) + p.buf.WriteString("}") + + if forcesNewResource { + p.buf.WriteString(p.color.Color(forcesNewResourceCaption)) + } + return } } - // In all other cases, we just show the new and old values as-is - p.writeValue(old, plans.Delete, indent) - if new.IsNull() { - p.buf.WriteString(p.color.Color(" [dark_gray]->[reset] ")) - } else { - p.buf.WriteString(p.color.Color(" [yellow]->[reset] ")) + // Avoid printing null -> "known" + if !old.IsNull() { + // In all other cases, we just show the new and old values as-is + p.writeValue(old, plans.Delete, indent) + if new.IsNull() { + p.buf.WriteString(p.color.Color(" [dark_gray]->[reset] ")) + } else { + p.buf.WriteString(p.color.Color(" [yellow]->[reset] ")) + } } p.writeValue(new, plans.Create, indent) if p.pathForcesNewResource(path) { diff --git a/command/format/diff_test.go b/command/format/diff_test.go index bc0a3ad82..43d2002bb 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -309,13 +309,73 @@ func TestResourceChange_JSON(t *testing.T) { ~ id = "i-02ae66f368e8518a9" -> (known after apply) ~ json_field = jsonencode( ~ { - - aaa = "value" - } -> { - + aaa = "value" + aaa = "value" + bbb = "new_value" } ) } +`, + }, + "in-place update (from empty tuple)": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"aaa": []}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"aaa": ["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( + ~ { + ~ aaa = [ + + "value", + ] + } + ) + } +`, + }, + "in-place update (to empty tuple)": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"aaa": ["value"]}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"aaa": []}`), + }), + 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( + ~ { + ~ aaa = [ + - "value", + ] + } + ) + } `, }, "force-new update": { @@ -343,9 +403,7 @@ func TestResourceChange_JSON(t *testing.T) { ~ id = "i-02ae66f368e8518a9" -> (known after apply) ~ json_field = jsonencode( ~ { - - aaa = "value" - } -> { - + aaa = "value" + aaa = "value" + bbb = "new_value" } # forces replacement ) @@ -438,6 +496,241 @@ func TestResourceChange_JSON(t *testing.T) { } `, }, + "JSON list item removal": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`["first","second","third"]`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`["first","second"]`), + }), + 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( + ~ [ + "first", + "second", + - "third", + ] + ) + } +`, + }, + "JSON list item addition": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`["first","second"]`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`["first","second","third"]`), + }), + 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( + ~ [ + "first", + "second", + + "third", + ] + ) + } +`, + }, + "JSON list object addition": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"first":"111"}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"first":"111","second":"222"}`), + }), + 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( + ~ { + first = "111" + + second = "222" + } + ) + } +`, + }, + "JSON object with nested list": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{ + "Statement": ["first"] + }`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{ + "Statement": ["first", "second"] + }`), + }), + 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( + ~ { + ~ Statement = [ + "first", + + "second", + ] + } + ) + } +`, + }, + "JSON list of objects": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`[{"one": "111"}]`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`[{"one": "111"}, {"two": "222"}]`), + }), + 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( + ~ [ + { + one = "111" + }, + + { + + two = "222" + }, + ] + ) + } +`, + }, + "JSON object with list of objects": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"parent":[{"one": "111"}]}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"parent":[{"one": "111"}, {"two": "222"}]}`), + }), + 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( + ~ { + ~ parent = [ + { + one = "111" + }, + + { + + two = "222" + }, + ] + } + ) + } +`, + }, + "JSON object double nested lists": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"parent":[{"another_list": ["111"]}]}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"parent":[{"another_list": ["111", "222"]}]}`), + }), + 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( + ~ { + ~ parent = [ + ~ { + ~ another_list = [ + "111", + + "222", + ] + }, + ] + } + ) + } +`, + }, + // TODO: JSON with unknown values inside } runTestCases(t, testCases) } From 0dff8fe5e0f29d98ee2ebecfbe847f20612d8fa4 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Tue, 22 Jan 2019 16:49:49 +0000 Subject: [PATCH 2/4] Add failing test case for tuple --- command/format/diff_test.go | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 43d2002bb..0c34fc323 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -376,6 +376,41 @@ func TestResourceChange_JSON(t *testing.T) { } ) } +`, + }, + "in-place update (tuple of different types)": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"aaa": [42, {"foo":"bar"}, "value"]}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`{"aaa": [42, {"foo":"baz"}, "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( + ~ { + ~ aaa = [ + 42, + ~ { + ~ foo = "bar" -> "baz" + }, + "value", + ] + } + ) + } `, }, "force-new update": { @@ -730,7 +765,6 @@ func TestResourceChange_JSON(t *testing.T) { } `, }, - // TODO: JSON with unknown values inside } runTestCases(t, testCases) } From f3d1565d6f0498f16404d9d69df7e4c1af69dc1f Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 23 Jan 2019 11:15:46 +0000 Subject: [PATCH 3/4] command/format: Fix tuple diff formatting --- command/format/diff.go | 200 ++++++++++------------------------------- 1 file changed, 47 insertions(+), 153 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index 24026db0e..da5fabd26 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -132,11 +132,6 @@ func ResourceChange( return buf.String() } -type ctyValueDiff struct { - Action plans.Action - Value cty.Value -} - type blockBodyDiffPrinter struct { buf *bytes.Buffer color *colorstring.Colorize @@ -648,31 +643,21 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa diffLines := ctySequenceDiff(oldLines, newLines) for _, diffLine := range diffLines { - line := diffLine.Value.AsString() + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.writeActionSymbol(diffLine.Action) + switch diffLine.Action { + case plans.NoOp, plans.Delete: + p.buf.WriteString(diffLine.Before.AsString()) case plans.Create: - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.buf.WriteString(p.color.Color("[green]+[reset] ")) - p.buf.WriteString(line) - p.buf.WriteString("\n") - case plans.Delete: - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.buf.WriteString(p.color.Color("[red]-[reset] ")) - p.buf.WriteString(line) - p.buf.WriteString("\n") - case plans.NoOp: - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.buf.WriteString(p.color.Color(" ")) - p.buf.WriteString(line) - p.buf.WriteString("\n") + p.buf.WriteString(diffLine.After.AsString()) default: // Should never happen since the above covers all - // actions that ctySequenceDiff can return. - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.buf.WriteString(p.color.Color("? ")) - p.buf.WriteString(line) - p.buf.WriteString("\n") + // actions that ctySequenceDiff can return for strings + p.buf.WriteString(diffLine.After.AsString()) + } + p.buf.WriteString("\n") } p.buf.WriteString(strings.Repeat(" ", indent)) // +4 here because there's no symbol @@ -752,27 +737,24 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa } p.buf.WriteString("\n") - if ty.IsTupleType() && ty.Length() > 0 && ty.TupleElementType(0).IsObjectType() { - elemDiffs := ctyObjectSequenceDiff(old.AsValueSlice(), new.AsValueSlice(), 0) - for _, elemDiff := range elemDiffs { - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.writeActionSymbol(elemDiff.Action) - if elemDiff.Action == plans.NoOp { - p.writeValue(elemDiff.Before, elemDiff.Action, indent+4) - } else { - p.writeValueDiff(elemDiff.Before, elemDiff.After, indent+4, path) - } + elemDiffs := ctySequenceDiff(old.AsValueSlice(), new.AsValueSlice()) + for _, elemDiff := range elemDiffs { + p.buf.WriteString(strings.Repeat(" ", indent+2)) + p.writeActionSymbol(elemDiff.Action) + switch elemDiff.Action { + case plans.NoOp, plans.Delete: + p.writeValue(elemDiff.Before, elemDiff.Action, indent+4) + case plans.Update: + p.writeValueDiff(elemDiff.Before, elemDiff.After, indent+4, path) + case plans.Create: + p.writeValue(elemDiff.After, elemDiff.Action, indent+4) + default: + // Should never happen since the above covers all + // actions that ctySequenceDiff can return. + p.writeValue(elemDiff.After, elemDiff.Action, indent+4) + } - p.buf.WriteString(",\n") - } - } else { - elemDiffs := ctySequenceDiff(old.AsValueSlice(), new.AsValueSlice()) - for _, elemDiff := range elemDiffs { - p.buf.WriteString(strings.Repeat(" ", indent+2)) - p.writeActionSymbol(elemDiff.Action) - p.writeValue(elemDiff.Value, elemDiff.Action, indent+4) - p.buf.WriteString(",\n") - } + p.buf.WriteString(",\n") } p.buf.WriteString(strings.Repeat(" ", indent)) @@ -934,16 +916,14 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa } } - // Avoid printing null -> "known" - if !old.IsNull() { - // In all other cases, we just show the new and old values as-is - p.writeValue(old, plans.Delete, indent) - if new.IsNull() { - p.buf.WriteString(p.color.Color(" [dark_gray]->[reset] ")) - } else { - p.buf.WriteString(p.color.Color(" [yellow]->[reset] ")) - } + // In all other cases, we just show the new and old values as-is + p.writeValue(old, plans.Delete, indent) + if new.IsNull() { + p.buf.WriteString(p.color.Color(" [dark_gray]->[reset] ")) + } else { + p.buf.WriteString(p.color.Color(" [yellow]->[reset] ")) } + p.writeValue(new, plans.Create, indent) if p.pathForcesNewResource(path) { p.buf.WriteString(p.color.Color(forcesNewResourceCaption)) @@ -1021,68 +1001,24 @@ func ctyCollectionValues(val cty.Value) []cty.Value { return ret } -func ctySequenceDiff(old, new []cty.Value) []ctyValueDiff { - var ret []ctyValueDiff - lcs := objchange.LongestCommonSubsequence(old, new) - var oldI, newI, lcsI int - for oldI < len(old) || newI < len(new) || lcsI < len(lcs) { - for oldI < len(old) && (lcsI >= len(lcs) || !old[oldI].RawEquals(lcs[lcsI])) { - ret = append(ret, ctyValueDiff{ - Action: plans.Delete, - Value: old[oldI], - }) - oldI++ - } - for newI < len(new) && (lcsI >= len(lcs) || !new[newI].RawEquals(lcs[lcsI])) { - ret = append(ret, ctyValueDiff{ - Action: plans.Create, - Value: new[newI], - }) - newI++ - } - if lcsI < len(lcs) { - ret = append(ret, ctyValueDiff{ - Action: plans.NoOp, - Value: new[newI], - }) - - // All of our indexes advance together now, since the line - // is common to all three sequences. - lcsI++ - oldI++ - newI++ - } - } - return ret -} - -// ctyObjectSequenceDiff is a variant of ctySequenceDiff that only works for -// values of object types. Whereas ctySequenceDiff can only return Create -// and Delete actions, this function can additionally return Update actions -// heuristically based on similarity of objects in the lists, which must -// be greater than or equal to the caller-specified threshold. -// -// See ctyObjectSimilarity for details on what "similarity" means here. -func ctyObjectSequenceDiff(old, new []cty.Value, threshold float64) []*plans.Change { +// ctySequenceDiff returns differences between given sequences of cty.Value(s) +// in the form of Create, Delete, or Update actions (for objects). +func ctySequenceDiff(old, new []cty.Value) []*plans.Change { var ret []*plans.Change lcs := objchange.LongestCommonSubsequence(old, new) var oldI, newI, lcsI int for oldI < len(old) || newI < len(new) || lcsI < len(lcs) { for oldI < len(old) && (lcsI >= len(lcs) || !old[oldI].RawEquals(lcs[lcsI])) { - if newI < len(new) { - // See if the next "new" is similar enough to our "old" that - // we'll treat this as an Update rather than a Delete/Create. - similarity := ctyObjectSimilarity(old[oldI], new[newI]) - if similarity >= threshold { - ret = append(ret, &plans.Change{ - Action: plans.Update, - Before: old[oldI], - After: new[newI], - }) - oldI++ - newI++ // we also consume the next "new" in this case - continue - } + isObjectDiff := old[oldI].Type().IsObjectType() && new[newI].Type().IsObjectType() + if isObjectDiff && newI < len(new) { + ret = append(ret, &plans.Change{ + Action: plans.Update, + Before: old[oldI], + After: new[newI], + }) + oldI++ + newI++ // we also consume the next "new" in this case + continue } ret = append(ret, &plans.Change{ @@ -1117,48 +1053,6 @@ func ctyObjectSequenceDiff(old, new []cty.Value, threshold float64) []*plans.Cha return ret } -// ctyObjectSimilarity returns a number between 0 and 1 that describes -// approximately how similar the two given values are, comparing in terms of -// how many of the corresponding attributes have the same value in both -// objects. -// -// This function expects the two values to have a similar set of attribute -// names, though doesn't mind if the two slightly differ since it will -// count missing attributes as differences. -// -// This function will panic if either of the given values is not an object. -func ctyObjectSimilarity(old, new cty.Value) float64 { - oldType := old.Type() - newType := new.Type() - attrNames := make(map[string]struct{}) - for name := range oldType.AttributeTypes() { - attrNames[name] = struct{}{} - } - for name := range newType.AttributeTypes() { - attrNames[name] = struct{}{} - } - - matches := 0 - - for name := range attrNames { - if !oldType.HasAttribute(name) { - continue - } - if !newType.HasAttribute(name) { - continue - } - eq := old.GetAttr(name).Equals(new.GetAttr(name)) - if !eq.IsKnown() { - continue - } - if eq.True() { - matches++ - } - } - - return float64(matches) / float64(len(attrNames)) -} - func ctyEqualWithUnknown(old, new cty.Value) bool { if !old.IsWhollyKnown() || !new.IsWhollyKnown() { return false From 953eae7e4bf3d7203c5463c97bddaef48aad4774 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 23 Jan 2019 13:13:48 +0000 Subject: [PATCH 4/4] command/format: Fix rendering of different types --- command/format/diff.go | 16 ++++++++++++++- command/format/diff_test.go | 41 ++++++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/command/format/diff.go b/command/format/diff.go index da5fabd26..049f62543 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -556,6 +556,7 @@ func (p *blockBodyDiffPrinter) writeValue(val cty.Value, action plans.Action, in func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, path cty.Path) { ty := old.Type() + typesEqual := ctyTypesEqual(ty, new.Type()) // We have some specialized diff implementations for certain complex // values where it's useful to see a visualization of the diff of @@ -563,7 +564,7 @@ func (p *blockBodyDiffPrinter) writeValueDiff(old, new cty.Value, indent int, pa // new values verbatim. // However, these specialized implementations can apply only if both // values are known and non-null. - if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() { + if old.IsKnown() && new.IsKnown() && !old.IsNull() && !new.IsNull() && typesEqual { switch { case ty == cty.String: // We have special behavior for both multi-line strings in general @@ -1060,6 +1061,19 @@ func ctyEqualWithUnknown(old, new cty.Value) bool { return old.Equals(new).True() } +// ctyTypesEqual checks equality of two types more loosely +// by avoiding checks of object/tuple elements +// as we render differences on element-by-element basis anyway +func ctyTypesEqual(oldT, newT cty.Type) bool { + if oldT.IsObjectType() && newT.IsObjectType() { + return true + } + if oldT.IsTupleType() && newT.IsTupleType() { + return true + } + return oldT.Equals(newT) +} + func ctyEnsurePathCapacity(path cty.Path, minExtra int) cty.Path { if cap(path)-len(path) >= minExtra { return path diff --git a/command/format/diff_test.go b/command/format/diff_test.go index 0c34fc323..5088e7631 100644 --- a/command/format/diff_test.go +++ b/command/format/diff_test.go @@ -286,7 +286,7 @@ func TestResourceChange_JSON(t *testing.T) { } `, }, - "in-place update": { + "in-place update of object": { Action: plans.Update, Mode: addrs.ManagedResourceMode, Before: cty.ObjectVal(map[string]cty.Value{ @@ -763,6 +763,45 @@ func TestResourceChange_JSON(t *testing.T) { } ) } +`, + }, + "in-place update from object to tuple": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "json_field": cty.StringVal(`{"aaa": [42, {"foo":"bar"}, "value"]}`), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "json_field": cty.StringVal(`["aaa", 42, "something"]`), + }), + 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( + ~ { + - aaa = [ + - 42, + - { + - foo = "bar" + }, + - "value", + ] + } -> [ + + "aaa", + + 42, + + "something", + ] + ) + } `, }, }