From 09dde8d5ed737efaf1be00c7d533e68e39a17cbe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Mar 2022 08:59:17 -0400 Subject: [PATCH] improve the contributing attributes filter The initial rough implementation contained a bug where it would incorrectly return a NilVal in some cases. Improve the heuristics here to insert null values more precisely when parent objects change to or from null. We also check for dynamic types changing, in which case the entire object must be taken when we can't match the individual attribute values. --- internal/command/views/plan.go | 36 +++- internal/command/views/plan_test.go | 266 ++++++++++++++++++++++++++++ 2 files changed, 294 insertions(+), 8 deletions(-) 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) + } + }) + } +}