customdiff: Use warning log instead of returning errors in ComputedIf, ForceNewIf, and ForceNewIfChange (#909)

Reference: hashicorp/terraform-plugin-sdk#908

Author: bflad

.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
+```

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

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

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

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

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"