From ef9d2e2a48c0cf444013a79088a81a811cb9d41b Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 17 Mar 2022 13:44:03 -0400 Subject: [PATCH] customdiff: Use warning log instead of returning errors in `ComputedIf`, `ForceNewIf`, and `ForceNewIfChange` (#909) Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/908 --- .changelog/909.txt | 3 ++ helper/customdiff/computed.go | 16 +++++- helper/customdiff/computed_test.go | 38 +++++++++++++++ helper/customdiff/force_new.go | 30 ++++++++++-- helper/customdiff/force_new_test.go | 76 +++++++++++++++++++++++++++++ internal/logging/keys.go | 4 ++ 6 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 .changelog/909.txt diff --git a/.changelog/909.txt b/.changelog/909.txt new file mode 100644 index 00000000000..45351958192 --- /dev/null +++ b/.changelog/909.txt @@ -0,0 +1,3 @@ +```release-note:bug +customdiff: Prevented unexpected non-existent key errors in `ComputedIf`, `ForceNewIf`, and `ForceNewIfChange` since 2.11.0, using a warning log for backwards compatibility instead +``` diff --git a/helper/customdiff/computed.go b/helper/customdiff/computed.go index 8a6ea358af2..c65a56401cc 100644 --- a/helper/customdiff/computed.go +++ b/helper/customdiff/computed.go @@ -2,18 +2,30 @@ package customdiff import ( "context" - "fmt" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging" ) // ComputedIf returns a CustomizeDiffFunc that sets the given key's new value // as computed if the given condition function returns true. +// +// This function is best effort and will generate a warning log on any errors. func ComputedIf(key string, f ResourceConditionFunc) schema.CustomizeDiffFunc { return func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { if f(ctx, d, meta) { + // To prevent backwards compatibility issues, this logic only + // generates a warning log instead of returning the error to + // the provider and ultimately the practitioner. Providers may + // not be aware of all situations in which the key may not be + // present in the data, such as during resource creation, so any + // further changes here should take that into account by + // documenting how to prevent the error. if err := d.SetNewComputed(key); err != nil { - return fmt.Errorf("unable to set %q to unknown: %w", key, err) + logging.HelperSchemaWarn(ctx, "unable to set attribute value to unknown", map[string]interface{}{ + logging.KeyAttributePath: key, + logging.KeyError: err, + }) } } return nil diff --git a/helper/customdiff/computed_test.go b/helper/customdiff/computed_test.go index 4fe394b1b09..e7c505d2e7b 100644 --- a/helper/customdiff/computed_test.go +++ b/helper/customdiff/computed_test.go @@ -8,6 +8,8 @@ import ( ) func TestComputedIf(t *testing.T) { + t.Parallel() + t.Run("true", func(t *testing.T) { var condCalls int var gotOld, gotNew string @@ -69,6 +71,24 @@ func TestComputedIf(t *testing.T) { t.Error("Attribute 'comp' is not marked as NewComputed") } }) + t.Run("true-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ComputedIf("non-existent", func(_ context.Context, d *schema.ResourceDiff, meta interface{}) bool { + return true + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) t.Run("false", func(t *testing.T) { var condCalls int var gotOld, gotNew string @@ -124,4 +144,22 @@ func TestComputedIf(t *testing.T) { t.Error("Attribute 'foo' is marked as NewComputed, but should not be") } }) + t.Run("false-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ComputedIf("non-existent", func(_ context.Context, d *schema.ResourceDiff, meta interface{}) bool { + return false + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) } diff --git a/helper/customdiff/force_new.go b/helper/customdiff/force_new.go index 9730e99fb73..f94257b7303 100644 --- a/helper/customdiff/force_new.go +++ b/helper/customdiff/force_new.go @@ -2,9 +2,9 @@ package customdiff import ( "context" - "fmt" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging" ) // ForceNewIf returns a CustomizeDiffFunc that flags the given key as @@ -13,11 +13,23 @@ import ( // The return value of the condition function is ignored if the old and new // values of the field compare equal, since no attribute diff is generated in // that case. +// +// This function is best effort and will generate a warning log on any errors. func ForceNewIf(key string, f ResourceConditionFunc) schema.CustomizeDiffFunc { return func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { if f(ctx, d, meta) { + // To prevent backwards compatibility issues, this logic only + // generates a warning log instead of returning the error to + // the provider and ultimately the practitioner. Providers may + // not be aware of all situations in which the key may not be + // present in the data, such as during resource creation, so any + // further changes here should take that into account by + // documenting how to prevent the error. if err := d.ForceNew(key); err != nil { - return fmt.Errorf("unable to set %q to require replacement: %w", key, err) + logging.HelperSchemaWarn(ctx, "unable to require attribute replacement", map[string]interface{}{ + logging.KeyAttributePath: key, + logging.KeyError: err, + }) } } return nil @@ -34,12 +46,24 @@ func ForceNewIf(key string, f ResourceConditionFunc) schema.CustomizeDiffFunc { // only the old and new values of the given key, which leads to more compact // and explicit code in the common case where the decision can be made with // only the specific field value. +// +// This function is best effort and will generate a warning log on any errors. func ForceNewIfChange(key string, f ValueChangeConditionFunc) schema.CustomizeDiffFunc { return func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { oldValue, newValue := d.GetChange(key) if f(ctx, oldValue, newValue, meta) { + // To prevent backwards compatibility issues, this logic only + // generates a warning log instead of returning the error to + // the provider and ultimately the practitioner. Providers may + // not be aware of all situations in which the key may not be + // present in the data, such as during resource creation, so any + // further changes here should take that into account by + // documenting how to prevent the error. if err := d.ForceNew(key); err != nil { - return fmt.Errorf("unable to set %q to require replacement: %w", key, err) + logging.HelperSchemaWarn(ctx, "unable to require attribute replacement", map[string]interface{}{ + logging.KeyAttributePath: key, + logging.KeyError: err, + }) } } return nil diff --git a/helper/customdiff/force_new_test.go b/helper/customdiff/force_new_test.go index f225feeeedf..3441761c59d 100644 --- a/helper/customdiff/force_new_test.go +++ b/helper/customdiff/force_new_test.go @@ -8,6 +8,8 @@ import ( ) func TestForceNewIf(t *testing.T) { + t.Parallel() + t.Run("true", func(t *testing.T) { var condCalls int var gotOld1, gotNew1, gotOld2, gotNew2 string @@ -77,6 +79,24 @@ func TestForceNewIf(t *testing.T) { t.Error("Attribute 'foo' is not marked as RequiresNew") } }) + t.Run("true-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ForceNewIf("non-existent", func(_ context.Context, d *schema.ResourceDiff, meta interface{}) bool { + return true + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) t.Run("false", func(t *testing.T) { var condCalls int var gotOld, gotNew string @@ -127,9 +147,29 @@ func TestForceNewIf(t *testing.T) { t.Error("Attribute 'foo' is marked as RequiresNew, but should not be") } }) + t.Run("false-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ForceNewIf("non-existent", func(_ context.Context, d *schema.ResourceDiff, meta interface{}) bool { + return false + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) } func TestForceNewIfChange(t *testing.T) { + t.Parallel() + t.Run("true", func(t *testing.T) { var condCalls int var gotOld1, gotNew1, gotOld2, gotNew2 string @@ -198,6 +238,24 @@ func TestForceNewIfChange(t *testing.T) { t.Error("Attribute 'foo' is not marked as RequiresNew") } }) + t.Run("true-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ForceNewIfChange("foo", func(_ context.Context, oldValue, newValue, meta interface{}) bool { + return true + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) t.Run("false", func(t *testing.T) { var condCalls int var gotOld, gotNew string @@ -247,4 +305,22 @@ func TestForceNewIfChange(t *testing.T) { t.Error("Attribute 'foo' is marked as RequiresNew, but should not be") } }) + t.Run("false-non-existent-attribute", func(t *testing.T) { + provider := testProvider( + map[string]*schema.Schema{}, + ForceNewIfChange("foo", func(_ context.Context, oldValue, newValue, meta interface{}) bool { + return true + }), + ) + + _, err := testDiff( + provider, + map[string]string{}, + map[string]string{}, + ) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) } diff --git a/internal/logging/keys.go b/internal/logging/keys.go index 5b8ad86d63b..5cc6068010d 100644 --- a/internal/logging/keys.go +++ b/internal/logging/keys.go @@ -8,6 +8,10 @@ package logging // Refer to the terraform-plugin-go logging keys as well, which should be // equivalent to these when possible. const ( + // Attribute path representation, which is typically in flatmap form such + // as parent.0.child in this project. + KeyAttributePath = "tf_attribute_path" + // The type of data source being operated on, such as "archive_file" KeyDataSourceType = "tf_data_source_type"