diff --git a/internal/command/views/plan.go b/internal/command/views/plan.go index 37bd7dfa66..488bfcc0f0 100644 --- a/internal/command/views/plan.go +++ b/internal/command/views/plan.go @@ -487,25 +487,45 @@ func filterRefreshChange(change *plans.ResourceInstanceChange, contributing []gl // We have some attributes in this change which were marked as relevant, so // we are going to take the Before value and add in only those attributes // from the After value which may have contributed to the plan. + + // If the types don't match because the schema is dynamic, we may not be + // able to apply the paths to the new values. + // if we encounter a path that does not apply correctly and the types do + // not match overall, just assume we need the entire value. + isDynamic := !change.Before.Type().Equals(change.After.Type()) + failedApply := false + before := change.Before after, _ := cty.Transform(before, func(path cty.Path, v cty.Value) (cty.Value, error) { for i, attrPath := range relevantAttrs { - // If the current value is null, but we are only a prefix of the - // affected path, we need to take the value from this point since - // we can't recurse any further into the object. This has the - // possibility of pulling in extra attribute changes we're not - // concerned with, but we can take this as "close enough" for now. - if (v.IsNull() && attrPath.HasPrefix(path)) || attrPath.Equals(path) { + // We match prefix in case we are traversing any null or dynamic + // values and enter in via a shorter path. The traversal is + // depth-first, so we will always hit the longest match first. + if attrPath.HasPrefix(path) { // remove the path from further consideration relevantAttrs = append(relevantAttrs[:i], relevantAttrs[i+1:]...) - v, err := path.Apply(change.After) - return v, err + applied, err := path.Apply(change.After) + if err != nil { + failedApply = true + // Assume the types match for now, and failure to apply is + // because a parent value is null. If there were dynamic + // types we'll just restore the entire value. + return cty.NullVal(v.Type()), nil + } + + return applied, err } } return v, nil }) + // A contributing attribute path did not match the after value type in some + // way, so restore the entire change. + if isDynamic && failedApply { + after = change.After + } + action := change.Action if before.RawEquals(after) { action = plans.NoOp diff --git a/internal/command/views/plan_test.go b/internal/command/views/plan_test.go index 757c7162dd..65e277595f 100644 --- a/internal/command/views/plan_test.go +++ b/internal/command/views/plan_test.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/command/arguments" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/lang/globalref" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/terminal" @@ -125,3 +126,268 @@ func testProviderSchema() *providers.GetProviderSchemaResponse { }, } } + +func TestFilterRefreshChange(t *testing.T) { + tests := map[string]struct { + paths []cty.Path + before, after, expected cty.Value + }{ + "attr was null": { + // nested attr was null + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("attr_null_before").GetAttr("b"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "attr_null_before": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("old"), + "b": cty.NullVal(cty.String), + }), + }), + }), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "attr_null_before": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + }), + }), + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "attr_null_before": cty.ObjectVal(map[string]cty.Value{ + // we old picked the change in b + "a": cty.StringVal("old"), + "b": cty.StringVal("new"), + }), + }), + }), + }, + "object was null": { + // nested object attrs were null + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("obj_null_before").GetAttr("b"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_before": cty.NullVal(cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.String, + })), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("old"), + }), + }), + }), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_before": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("new"), + }), + }), + }), + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_before": cty.ObjectVal(map[string]cty.Value{ + // optimally "a" would be null, but we need to take the + // entire object since it was null before. + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("old"), + }), + }), + }), + }, + "object becomes null": { + // nested object attr becoming null + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("obj_null_after").GetAttr("a"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("old"), + "b": cty.StringVal("old"), + }), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("old"), + }), + }), + }), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_after": cty.NullVal(cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.String, + })), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("new"), + }), + }), + }), + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "obj_null_after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.NullVal(cty.String), + "b": cty.StringVal("old"), + }), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("old"), + }), + }), + }), + }, + "dynamic adding values": { + // dynamic gaining values + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("after").GetAttr("a"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.DynamicVal, + }), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + // the entire attr object is taken here because there is + // nothing to compare within the before value + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("new"), + }), + }), + }), + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + // "other" is picked up here too this time, because we need + // to take the entire dynamic "attr" value + "other": cty.ObjectVal(map[string]cty.Value{ + "o": cty.StringVal("new"), + }), + }), + }), + }, + "whole object becomes null": { + // whole object becomes null + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("after").GetAttr("a"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("old"), + "b": cty.StringVal("old"), + }), + }), + }), + after: cty.NullVal(cty.Object(map[string]cty.Type{ + "attr": cty.DynamicPseudoType, + })), + // since we have a dynamic type we have to take the entire object + // because the paths may not apply between versions. + expected: cty.NullVal(cty.Object(map[string]cty.Type{ + "attr": cty.DynamicPseudoType, + })), + }, + "whole object was null": { + // whole object was null + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("after").GetAttr("a"), + }, + before: cty.NullVal(cty.Object(map[string]cty.Type{ + "attr": cty.DynamicPseudoType, + })), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + }), + }), + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + }), + }), + }, + "restructured dynamic": { + // dynamic value changing structure significantly + paths: []cty.Path{ + cty.GetAttrPath("attr").GetAttr("list").IndexInt(1).GetAttr("a"), + }, + before: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("old"), + }), + }), + }), + }), + after: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + }), + }), + // the path does not apply at all to the new object, so we must + // take all the changes + expected: cty.ObjectVal(map[string]cty.Value{ + "attr": cty.ObjectVal(map[string]cty.Value{ + "after": cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("new"), + "b": cty.StringVal("new"), + }), + }), + }), + }, + } + + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + addr, diags := addrs.ParseAbsResourceInstanceStr("test_resource.a") + if diags != nil { + t.Fatal(diags.ErrWithWarnings()) + } + + change := &plans.ResourceInstanceChange{ + Addr: addr, + Change: plans.Change{ + Before: tc.before, + After: tc.after, + Action: plans.Update, + }, + } + + var contributing []globalref.ResourceAttr + for _, p := range tc.paths { + contributing = append(contributing, globalref.ResourceAttr{ + Resource: addr, + Attr: p, + }) + } + + res := filterRefreshChange(change, contributing) + if !res.After.RawEquals(tc.expected) { + t.Errorf("\nexpected: %#v\ngot: %#v\n", tc.expected, res.After) + } + }) + } +}