Merge pull request #30765 from hashicorp/jbardin/contributing-attributes

improve the contributing attributes filter

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
+			// We match prefix in case we are traversing any null or dynamic
-			// affected path, we need to take the value from this point since
+			// values and enter in via a shorter path. The traversal is
-			// we can't recurse any further into the object. This has the
+			// depth-first, so we will always hit the longest match first.
-			// possibility of pulling in extra attribute changes we're not
+			if attrPath.HasPrefix(path) {
-			// concerned with, but we can take this as "close enough" for now.
-			if (v.IsNull() && attrPath.HasPrefix(path)) || attrPath.Equals(path) {
 				// remove the path from further consideration
 				relevantAttrs = append(relevantAttrs[:i], relevantAttrs[i+1:]...)
 
-				v, err := path.Apply(change.After)
+				applied, err := path.Apply(change.After)
-				return v, err
+				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

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)
+			}
+		})
+	}
+}