From 486e1dcb9d64323e124567919bed9887661f9926 Mon Sep 17 00:00:00 2001 From: Paddy Carver Date: Wed, 27 Oct 2021 10:32:25 -0700 Subject: [PATCH] Complicate our RequiresReplace(If) logic. Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants. --- tfsdk/attribute_plan_modification.go | 101 +++- tfsdk/attribute_plan_modification_test.go | 682 ++++++++++++++++++++++ tfsdk/attribute_test.go | 12 +- tfsdk/serve_test.go | 14 +- 4 files changed, 795 insertions(+), 14 deletions(-) create mode 100644 tfsdk/attribute_plan_modification_test.go diff --git a/tfsdk/attribute_plan_modification.go b/tfsdk/attribute_plan_modification.go index 8295f6972..cd6f30a0c 100644 --- a/tfsdk/attribute_plan_modification.go +++ b/tfsdk/attribute_plan_modification.go @@ -2,6 +2,7 @@ package tfsdk import ( "context" + "fmt" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" @@ -64,6 +65,52 @@ type RequiresReplaceModifier struct{} // Modify sets RequiresReplace on the response to true. func (r RequiresReplaceModifier) Modify(ctx context.Context, req ModifyAttributePlanRequest, resp *ModifyAttributePlanResponse) { + if req.AttributeConfig == nil || req.AttributePlan == nil || req.AttributeState == nil { + // shouldn't happen, but let's not panic if it does + return + } + + if req.State.Raw.IsNull() { + // if we're creating the resource, no need to delete and + // recreate it + return + } + + if req.Plan.Raw.IsNull() { + // if we're deleting the resource, no need to delete and + // recreate it + return + } + + attrSchema, err := req.State.Schema.AttributeAtPath(req.AttributePath) + if err != nil { + resp.Diagnostics.AddAttributeError(req.AttributePath, + "Error finding attribute schema", + fmt.Sprintf("An unexpected error was encountered retrieving the schema for this attribute. This is always a bug in the provider.\n\nError: %s", err), + ) + return + } + + configRaw, err := req.AttributeConfig.ToTerraformValue(ctx) + if err != nil { + resp.Diagnostics.AddAttributeError(req.AttributePath, + "Error converting config value", + fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representation. This is always a bug in the provider.\n\nError: %s", req.AttributeConfig.Type(ctx), err), + ) + return + } + if configRaw == nil && attrSchema.Computed { + // if the config is null and the attribute is computed, this + // could be an out of band change, don't require replace + return + } + + if req.AttributePlan.Equal(req.AttributeState) { + // if the plan and the state are in agreement, this attribute + // isn't changing, don't require replace + return + } + resp.RequiresReplace = true } @@ -103,9 +150,61 @@ type RequiresReplaceIfModifier struct { // Modify sets RequiresReplace on the response to true if the conditional // RequiresReplaceIfFunc returns true. func (r RequiresReplaceIfModifier) Modify(ctx context.Context, req ModifyAttributePlanRequest, resp *ModifyAttributePlanResponse) { + if req.AttributeConfig == nil || req.AttributePlan == nil || req.AttributeState == nil { + // shouldn't happen, but let's not panic if it does + return + } + + if req.State.Raw.IsNull() { + // if we're creating the resource, no need to delete and + // recreate it + return + } + + if req.Plan.Raw.IsNull() { + // if we're deleting the resource, no need to delete and + // recreate it + return + } + + attrSchema, err := req.State.Schema.AttributeAtPath(req.AttributePath) + if err != nil { + resp.Diagnostics.AddAttributeError(req.AttributePath, + "Error finding attribute schema", + fmt.Sprintf("An unexpected error was encountered retrieving the schema for this attribute. This is always a bug in the provider.\n\nError: %s", err), + ) + return + } + + configRaw, err := req.AttributeConfig.ToTerraformValue(ctx) + if err != nil { + resp.Diagnostics.AddAttributeError(req.AttributePath, + "Error converting config value", + fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representation. This is always a bug in the provider.\n\nError: %s", req.AttributeConfig.Type(ctx), err), + ) + return + } + if configRaw == nil && attrSchema.Computed { + // if the config is null and the attribute is computed, this + // could be an out of band change, don't require replace + return + } + + if req.AttributePlan.Equal(req.AttributeState) { + // if the plan and the state are in agreement, this attribute + // isn't changing, don't require replace + return + } + res, diags := r.f(ctx, req.AttributeState, req.AttributeConfig, req.AttributePath) resp.Diagnostics.Append(diags...) - resp.RequiresReplace = res + + // if the function says to require replacing, we require replacing + // if the function says not to, we don't change the value that other + // plan modifiers may have set + if res { + resp.RequiresReplace = true + } } // Description returns a human-readable description of the plan modifier. diff --git a/tfsdk/attribute_plan_modification_test.go b/tfsdk/attribute_plan_modification_test.go new file mode 100644 index 000000000..554b1056e --- /dev/null +++ b/tfsdk/attribute_plan_modification_test.go @@ -0,0 +1,682 @@ +package tfsdk + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-plugin-framework/attr" + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-go/tftypes" +) + +func TestRequiresReplaceModifier(t *testing.T) { + t.Parallel() + + type testCase struct { + state State + plan Plan + config Config + path *tftypes.AttributePath + expectedPlan attr.Value + expectedRR bool + } + + schema := Schema{ + Attributes: map[string]Attribute{ + "a": { + Type: types.StringType, + Optional: true, + Computed: true, + }, + "b": { + Type: types.StringType, + Optional: true, + }, + }, + } + + tests := map[string]testCase{ + "null-state": { + // when we first create the resource, it shouldn't + // require replacing immediately + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + expectedPlan: types.String{Value: "foo"}, + expectedRR: false, + }, + "null-plan": { + // when we destroy the resource, it shouldn't require + // replacing + // + // Terraform doesn't usually ask for provider input on + // the plan when destroying resources, but in case it + // does, let's make sure we handle it right + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + expectedPlan: nil, + expectedRR: false, + }, + "null-attribute-state": { + // make sure we're not confusing an attribute going + // from null to a value with the resource getting + // created + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + expectedPlan: types.String{Value: "bar"}, + expectedRR: true, + }, + "null-attribute-plan": { + // make sure we're not confusing an attribute going + // from a value to null with the resource getting + // destroyed + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + expectedPlan: types.String{Null: true}, + expectedRR: true, + }, + "known-state-change": { + // when updating the attribute, if it has changed, it + // should require replacing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + expectedPlan: types.String{Value: "quux"}, + expectedRR: true, + }, + "known-state-no-change": { + // when the attribute hasn't changed, it shouldn't + // require replacing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + expectedPlan: types.String{Value: "foo"}, + expectedRR: false, + }, + "null-config-computed": { + // if the config is null for a computed attribute, we + // shouldn't require replacing, even if it's a change. + // + // this is sometimes unintuitive, if the practitioner + // is changing it on purpose. However, it's + // indistinguishable from the provider changing it, and + // practitioners pretty much never expect the resource + // to be recreated if the provider is the one changing + // the value. + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, tftypes.UnknownValue), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, nil), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + expectedPlan: types.String{Unknown: true}, + expectedRR: false, + }, + "null-config-not-computed": { + // if the config is null for a non-computed attribute, + // we should require replacing if it's a change. + // + // unlike computed attributes, this is always a + // practitioner making a change, and therefore the + // destroy/recreate cycle is likely expected. + // + // this test is technically covered by + // null-attribute-plan, but let's duplicate it just to + // be explicit about what each case is actually testing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + expectedPlan: types.String{Null: true}, + expectedRR: true, + }, + } + + for name, tc := range tests { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + attrConfig, diags := tc.config.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + attrState, diags := tc.state.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + attrPlan, diags := tc.plan.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + req := ModifyAttributePlanRequest{ + AttributePath: tc.path, + Config: tc.config, + State: tc.state, + Plan: tc.plan, + AttributeConfig: attrConfig, + AttributeState: attrState, + AttributePlan: attrPlan, + ProviderMeta: Config{}, + } + resp := &ModifyAttributePlanResponse{ + AttributePlan: req.AttributePlan, + } + modifier := RequiresReplace() + + modifier.Modify(context.Background(), req, resp) + if resp.Diagnostics.HasError() { + t.Fatalf("Unexpected diagnostics: %s", resp.Diagnostics) + } + if diff := cmp.Diff(tc.expectedPlan, resp.AttributePlan); diff != "" { + t.Fatalf("Unexpected diff in plan (-wanted, +got): %s", diff) + } + if diff := cmp.Diff(tc.expectedRR, resp.RequiresReplace); diff != "" { + t.Fatalf("Unexpected diff in RequiresReplace (-wanted, +got): %s", diff) + } + }) + } +} + +func TestRequiresReplaceIfModifier(t *testing.T) { + t.Parallel() + + type testCase struct { + state State + plan Plan + config Config + path *tftypes.AttributePath + ifReturn bool + expectedPlan attr.Value + expectedRR bool + } + + schema := Schema{ + Attributes: map[string]Attribute{ + "a": { + Type: types.StringType, + Optional: true, + Computed: true, + }, + "b": { + Type: types.StringType, + Optional: true, + }, + }, + } + + tests := map[string]testCase{ + "null-state": { + // when we first create the resource, it shouldn't + // require replacing immediately + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + ifReturn: true, + expectedPlan: types.String{Value: "foo"}, + expectedRR: false, + }, + "null-plan": { + // when we destroy the resource, it shouldn't require + // replacing + // + // Terraform doesn't usually ask for provider input on + // the plan when destroying resources, but in case it + // does, let's make sure we handle it right + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), nil), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + ifReturn: true, + expectedPlan: nil, + expectedRR: false, + }, + "null-attribute-state": { + // make sure we're not confusing an attribute going + // from null to a value with the resource getting + // created + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + ifReturn: true, + expectedPlan: types.String{Value: "bar"}, + expectedRR: true, + }, + "null-attribute-plan": { + // make sure we're not confusing an attribute going + // from a value to null with the resource getting + // destroyed + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + ifReturn: true, + path: tftypes.NewAttributePath().WithAttributeName("b"), + expectedPlan: types.String{Null: true}, + expectedRR: true, + }, + "known-state-change-true": { + // when updating the attribute, if it has changed and + // the function returns true, it should require + // replacing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + ifReturn: true, + expectedPlan: types.String{Value: "quux"}, + expectedRR: true, + }, + "known-state-change-false": { + // when updating the attribute, if it has changed and + // the function returns false, it should not require + // replacing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + ifReturn: false, + expectedPlan: types.String{Value: "quux"}, + expectedRR: false, + }, + "known-state-no-change": { + // when the attribute hasn't changed, it shouldn't + // require replacing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + ifReturn: true, + expectedPlan: types.String{Value: "foo"}, + expectedRR: false, + }, + "null-config-computed": { + // if the config is null for a computed attribute, we + // shouldn't require replacing, even if it's a change. + // + // this is sometimes unintuitive, if the practitioner + // is changing it on purpose. However, it's + // indistinguishable from the provider changing it, and + // practitioners pretty much never expect the resource + // to be recreated if the provider is the one changing + // the value. + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, tftypes.UnknownValue), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, nil), + "b": tftypes.NewValue(tftypes.String, "quux"), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("a"), + ifReturn: true, + expectedPlan: types.String{Unknown: true}, + expectedRR: false, + }, + "null-config-not-computed": { + // if the config is null for a non-computed attribute, + // we should require replacing if it's a change. + // + // unlike computed attributes, this is always a + // practitioner making a change, and therefore the + // destroy/recreate cycle is likely expected. + // + // this test is technically covered by + // null-attribute-plan, but let's duplicate it just to + // be explicit about what each case is actually testing + state: State{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, "bar"), + }), + }, + plan: Plan{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + config: Config{ + Schema: schema, + Raw: tftypes.NewValue(schema.TerraformType(context.Background()), map[string]tftypes.Value{ + "a": tftypes.NewValue(tftypes.String, "foo"), + "b": tftypes.NewValue(tftypes.String, nil), + }), + }, + path: tftypes.NewAttributePath().WithAttributeName("b"), + ifReturn: true, + expectedPlan: types.String{Null: true}, + expectedRR: true, + }, + } + + for name, tc := range tests { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + attrConfig, diags := tc.config.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + attrState, diags := tc.state.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + attrPlan, diags := tc.plan.GetAttribute(context.Background(), tc.path) + if diags.HasError() { + t.Fatalf("Got unexpected diagnostics: %s", diags) + } + + req := ModifyAttributePlanRequest{ + AttributePath: tc.path, + Config: tc.config, + State: tc.state, + Plan: tc.plan, + AttributeConfig: attrConfig, + AttributeState: attrState, + AttributePlan: attrPlan, + ProviderMeta: Config{}, + } + resp := &ModifyAttributePlanResponse{ + AttributePlan: req.AttributePlan, + } + modifier := RequiresReplaceIf(func(ctx context.Context, state, config attr.Value, path *tftypes.AttributePath) (bool, diag.Diagnostics) { + return tc.ifReturn, nil + }, "", "") + + modifier.Modify(context.Background(), req, resp) + if resp.Diagnostics.HasError() { + t.Fatalf("Unexpected diagnostics: %s", resp.Diagnostics) + } + if diff := cmp.Diff(tc.expectedPlan, resp.AttributePlan); diff != "" { + t.Fatalf("Unexpected diff in plan (-wanted, +got): %s", diff) + } + if diff := cmp.Diff(tc.expectedRR, resp.RequiresReplace); diff != "" { + t.Fatalf("Unexpected diff in RequiresReplace (-wanted, +got): %s", diff) + } + }) + } +} diff --git a/tfsdk/attribute_test.go b/tfsdk/attribute_test.go index c22ee81a5..6f721e8e8 100644 --- a/tfsdk/attribute_test.go +++ b/tfsdk/attribute_test.go @@ -1383,7 +1383,7 @@ func TestAttributeModifyPlan(t *testing.T) { "test": tftypes.String, }, }, map[string]tftypes.Value{ - "test": tftypes.NewValue(tftypes.String, "testvalue"), + "test": tftypes.NewValue(tftypes.String, "newtestvalue"), }), Schema: Schema{ Attributes: map[string]Attribute{ @@ -1403,7 +1403,7 @@ func TestAttributeModifyPlan(t *testing.T) { "test": tftypes.String, }, }, map[string]tftypes.Value{ - "test": tftypes.NewValue(tftypes.String, "testvalue"), + "test": tftypes.NewValue(tftypes.String, "newtestvalue"), }), Schema: Schema{ Attributes: map[string]Attribute{ @@ -1455,7 +1455,7 @@ func TestAttributeModifyPlan(t *testing.T) { "test": tftypes.String, }, }, map[string]tftypes.Value{ - "test": tftypes.NewValue(tftypes.String, "testvalue"), + "test": tftypes.NewValue(tftypes.String, "newtestvalue"), }), Schema: Schema{ Attributes: map[string]Attribute{ @@ -1475,7 +1475,7 @@ func TestAttributeModifyPlan(t *testing.T) { "test": tftypes.String, }, }, map[string]tftypes.Value{ - "test": tftypes.NewValue(tftypes.String, "testvalue"), + "test": tftypes.NewValue(tftypes.String, "newtestvalue"), }), Schema: Schema{ Attributes: map[string]Attribute{ @@ -1547,8 +1547,8 @@ func TestAttributeModifyPlan(t *testing.T) { Type: types.StringType, Required: true, PlanModifiers: []AttributePlanModifier{ - RequiresReplace(), testAttrPlanValueModifierOne{}, + RequiresReplace(), }, }, }, @@ -2041,7 +2041,7 @@ func TestAttributeModifyPlan(t *testing.T) { attribute.modifyPlan(context.Background(), tc.req, &tc.resp) if diff := cmp.Diff(tc.expectedResp, tc.resp); diff != "" { - t.Errorf("Unexpected response (+wanted, -got): %s", diff) + t.Errorf("Unexpected response (-wanted, +got): %s", diff) } }) } diff --git a/tfsdk/serve_test.go b/tfsdk/serve_test.go index 0bf02a1b3..c3817ca9d 100644 --- a/tfsdk/serve_test.go +++ b/tfsdk/serve_test.go @@ -2527,7 +2527,7 @@ func TestServerPlanResourceChange(t *testing.T) { "format": tftypes.String, }}, map[string]tftypes.Value{ "size": tftypes.NewValue(tftypes.Number, 1), - "format": tftypes.NewValue(tftypes.String, "ext4"), + "format": tftypes.NewValue(tftypes.String, "ext3"), }), }), "region": tftypes.NewValue(tftypes.String, "region1"), @@ -2626,7 +2626,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String, @@ -2715,7 +2715,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String, @@ -2804,7 +2804,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String, @@ -2905,7 +2905,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String, @@ -2994,7 +2994,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String, @@ -3174,7 +3174,7 @@ func TestServerPlanResourceChange(t *testing.T) { }}, }}, map[string]tftypes.Value{ "id": tftypes.NewValue(tftypes.String, "my-scr-disk"), - "interface": tftypes.NewValue(tftypes.String, "scsi"), + "interface": tftypes.NewValue(tftypes.String, "something-else"), "filesystem": tftypes.NewValue(tftypes.Object{AttributeTypes: map[string]tftypes.Type{ "size": tftypes.Number, "format": tftypes.String,