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

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